From 71f922c42d99534e527b3f7df53ec29f10692761 Mon Sep 17 00:00:00 2001 From: Ted Kaemming Date: Tue, 20 Aug 2019 17:41:35 -0700 Subject: [PATCH 1/5] subschemas --- snuba/api.py | 13 ++- snuba/schemas.py | 205 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 149 insertions(+), 69 deletions(-) diff --git a/snuba/api.py b/snuba/api.py index 0ce4a193913..08751480573 100644 --- a/snuba/api.py +++ b/snuba/api.py @@ -183,12 +183,14 @@ def parse_request_body(request): def validate_request_content(body, schema, timer): try: - schemas.validate(body, schema) + request = schema.validate(body) except jsonschema.ValidationError as error: raise BadRequest(str(error)) from error timer.mark('validate_schema') + return {**request.body} + @application.route('/query', methods=['GET', 'POST']) @util.time_request('query') @@ -226,7 +228,7 @@ def dataset_query(dataset, body, timer): assert request.method == 'POST' ensure_table_exists(dataset) - validate_request_content(body, dataset.get_query_schema(), timer) + body = validate_request_content(body, dataset.get_query_schema(), timer) result, status = parse_and_run_query(dataset, body, timer) return ( @@ -396,8 +398,11 @@ def parse_and_run_query(dataset, body, timer): @application.route('/internal/sdk-stats', methods=['POST']) @util.time_request('sdk-stats') def sdk_distribution(*, timer: Timer): - body = parse_request_body(request) - validate_request_content(body, schemas.SDK_STATS_SCHEMA, timer) + body = validate_request_content( + parse_request_body(request), + schemas.SDK_STATS_SCHEMA, + timer, + ) body['project'] = [] body['aggregations'] = [ diff --git a/snuba/schemas.py b/snuba/schemas.py index cd81d4bc10c..f4d73cbb3a4 100644 --- a/snuba/schemas.py +++ b/snuba/schemas.py @@ -7,44 +7,6 @@ POSITIVE_OPERATORS = ['>', '<', '>=', '<=', '=', 'IN', 'IS NULL', 'LIKE'] -def get_time_series_query_schema_properties(default_granularity: int, default_window: timedelta): - return { - 'from_date': { - 'type': 'string', - 'format': 'date-time', - 'default': lambda: (datetime.utcnow().replace(microsecond=0) - default_window).isoformat() - }, - 'to_date': { - 'type': 'string', - 'format': 'date-time', - 'default': lambda: datetime.utcnow().replace(microsecond=0).isoformat() - }, - 'granularity': { - 'type': 'number', - 'default': default_granularity, - }, - } - - -SDK_STATS_SCHEMA = { - 'type': 'object', - 'properties': { - 'groupby': { - 'type': 'array', - 'items': { - # at the moment the only additional thing you can group by is project_id - 'enum': ['project_id'] - }, - 'default': [], - }, - **get_time_series_query_schema_properties( - default_granularity=86400, # SDK stats query defaults to 1-day bucketing - default_window=timedelta(days=1), - ), - }, - 'additionalProperties': False, -} - GENERIC_QUERY_SCHEMA = { 'type': 'object', 'properties': { @@ -217,42 +179,155 @@ def get_time_series_query_schema_properties(default_granularity: int, default_wi } } -EVENTS_QUERY_SCHEMA = { - **copy.deepcopy(GENERIC_QUERY_SCHEMA), +PERFORMANCE_EXTENSION_SCHEMA = { + 'type': 'object', + 'properties': { + # Never add FINAL to queries, enable sampling + 'turbo': { + 'type': 'boolean', + 'default': False, + }, + # Force queries to hit the first shard replica, ensuring the query + # sees data that was written before the query. This burdens the + # first replica, so should only be used when absolutely necessary. + 'consistent': { + 'type': 'boolean', + 'default': False, + }, + 'debug': { + 'type': 'boolean', + 'default': False, + }, + }, + 'additionalProperties': False, +} + +PROJECT_EXTENSION_SCHEMA = { + 'type': 'object', + 'properties': { + 'project': { + 'anyOf': [ + {'type': 'number'}, + { + 'type': 'array', + 'items': {'type': 'number'}, + 'minItems': 1, + }, + ] + }, + }, # Need to select down to the project level for customer isolation and performance 'required': ['project'], + 'additionalProperties': False, } -EVENTS_QUERY_SCHEMA['properties'].update({ - **get_time_series_query_schema_properties( + +def get_time_series_extension_properties(default_granularity: int, default_window: timedelta): + return { + 'type': 'object', + 'properties': { + 'from_date': { + 'type': 'string', + 'format': 'date-time', + 'default': lambda: (datetime.utcnow().replace(microsecond=0) - default_window).isoformat() + }, + 'to_date': { + 'type': 'string', + 'format': 'date-time', + 'default': lambda: datetime.utcnow().replace(microsecond=0).isoformat() + }, + 'granularity': { + 'type': 'number', + 'default': default_granularity, + }, + }, + 'additionalProperties': False, + } + + +import itertools +from collections import ChainMap +from typing import Any, Mapping +from dataclasses import dataclass + + +@dataclass +class Request: + query: Mapping[str, Any] + extensions: Mapping[str, Mapping[str, Any]] + + def __post_init__(self): + self.body = ChainMap(self.query, *self.extensions.values()) + + +class RequestSchema: + def __init__(self, query_schema, extensions_schemas: Mapping[str, Any]): + self.__query_schema = query_schema + self.__extension_schemas = extensions_schemas + + self.__composite_schema = { + 'type': 'object', + 'properties': {}, + 'required': [], + 'definitions': {}, + 'additionalProperties': False, + } + + for schema in itertools.chain([self.__query_schema], self.__extension_schemas.values()): + assert schema['type'] == 'object', 'subschema must be object' + assert schema['additionalProperties'] is False, 'subschema must not allow additional properties' + self.__composite_schema['required'].extend(schema.get('required', [])) + + for property_name, property_schema in schema['properties'].items(): + assert property_name not in self.__composite_schema['properties'], 'subschema cannot redefine property' + self.__composite_schema['properties'][property_name] = property_schema + + for definition_name, definition_schema in schema.get('definitions', {}).items(): + assert definition_name not in self.__composite_schema['definitions'], 'subschema cannot redefine definition' + self.__composite_schema['definitions'][definition_name] = definition_schema + + self.__composite_schema['required'] = set(self.__composite_schema['required']) + + def validate(self, value) -> Request: + # XXX: Mutates input value! + validate(value, self.__composite_schema) + + query = {key: value.pop(key) for key in self.__query_schema['properties'].keys() if key in value} + + extensions = {} + for extension_name, extension_schema in self.__extension_schemas.items(): + extensions[extension_name] = {key: value.pop(key) for key in extension_schema['properties'].keys() if key in value} + + return Request(query, extensions) + + +EVENTS_QUERY_SCHEMA = RequestSchema(GENERIC_QUERY_SCHEMA, { + 'performance': PERFORMANCE_EXTENSION_SCHEMA, + 'project': PROJECT_EXTENSION_SCHEMA, + 'timeseries': get_time_series_extension_properties( default_granularity=3600, default_window=timedelta(days=5), ), - 'project': { - 'anyOf': [ - {'type': 'number'}, - { - 'type': 'array', - 'items': {'type': 'number'}, - 'minItems': 1, +}) + +SDK_STATS_SCHEMA = RequestSchema({ + 'type': 'object', + 'properties': { + 'groupby': { + 'type': 'array', + 'items': { + # at the moment the only additional thing you can group by is project_id + 'enum': ['project_id'] }, - ] - }, - # Never add FINAL to queries, enable sampling - 'turbo': { - 'type': 'boolean', - 'default': False, - }, - # Force queries to hit the first shard replica, ensuring the query - # sees data that was written before the query. This burdens the - # first replica, so should only be used when absolutely necessary. - 'consistent': { - 'type': 'boolean', - 'default': False, + 'default': [], + }, }, - 'debug': { - 'type': 'boolean', - } + 'additionalProperties': False, +}, { + 'timeseries': get_time_series_extension_properties( + default_granularity=86400, # SDK stats query defaults to 1-day bucketing + default_window=timedelta(days=1), + ), }) From 6aec0f3f8e463e2bba8dc7babd137781c1f08749 Mon Sep 17 00:00:00 2001 From: Ted Kaemming Date: Tue, 20 Aug 2019 17:51:21 -0700 Subject: [PATCH 2/5] tidy --- snuba/schemas.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/snuba/schemas.py b/snuba/schemas.py index f4d73cbb3a4..cf63d363edc 100644 --- a/snuba/schemas.py +++ b/snuba/schemas.py @@ -1,12 +1,16 @@ +import copy +import itertools +from collections import ChainMap +from dataclasses import dataclass from datetime import datetime, timedelta +from typing import Any, Mapping + import jsonschema -import copy CONDITION_OPERATORS = ['>', '<', '>=', '<=', '=', '!=', 'IN', 'NOT IN', 'IS NULL', 'IS NOT NULL', 'LIKE', 'NOT LIKE'] POSITIVE_OPERATORS = ['>', '<', '>=', '<=', '=', 'IN', 'IS NULL', 'LIKE'] - GENERIC_QUERY_SCHEMA = { 'type': 'object', 'properties': { @@ -245,12 +249,6 @@ def get_time_series_extension_properties(default_granularity: int, default_windo } -import itertools -from collections import ChainMap -from typing import Any, Mapping -from dataclasses import dataclass - - @dataclass class Request: query: Mapping[str, Any] From f85b70e2e8b5c1087ee0563cb583e70d9adc7963 Mon Sep 17 00:00:00 2001 From: Ted Kaemming Date: Wed, 21 Aug 2019 15:06:15 -0700 Subject: [PATCH 3/5] freeze Request --- snuba/schemas.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/snuba/schemas.py b/snuba/schemas.py index cf63d363edc..b846be1ec76 100644 --- a/snuba/schemas.py +++ b/snuba/schemas.py @@ -249,13 +249,14 @@ def get_time_series_extension_properties(default_granularity: int, default_windo } -@dataclass +@dataclass(frozen=True) class Request: query: Mapping[str, Any] extensions: Mapping[str, Mapping[str, Any]] - def __post_init__(self): - self.body = ChainMap(self.query, *self.extensions.values()) + @property + def body(self): + return ChainMap(self.query, *self.extensions.values()) class RequestSchema: From 188cbacef363fb3bc81c2533e5663b7b8a76a1c0 Mon Sep 17 00:00:00 2001 From: Ted Kaemming Date: Wed, 21 Aug 2019 15:12:35 -0700 Subject: [PATCH 4/5] add type stubs for schema --- snuba/schemas.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/snuba/schemas.py b/snuba/schemas.py index b846be1ec76..ef368be86b2 100644 --- a/snuba/schemas.py +++ b/snuba/schemas.py @@ -259,8 +259,11 @@ def body(self): return ChainMap(self.query, *self.extensions.values()) +Schema = Mapping[str, Any] # placeholder for JSON schema + + class RequestSchema: - def __init__(self, query_schema, extensions_schemas: Mapping[str, Any]): + def __init__(self, query_schema: Schema, extensions_schemas: Mapping[str, Schema]): self.__query_schema = query_schema self.__extension_schemas = extensions_schemas From 70b6e8bd3e38c6e3f28308ff3a43cc8cc480487c Mon Sep 17 00:00:00 2001 From: Ted Kaemming Date: Wed, 21 Aug 2019 15:12:48 -0700 Subject: [PATCH 5/5] don't mutate input value to schema validation --- snuba/schemas.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/snuba/schemas.py b/snuba/schemas.py index ef368be86b2..609459570a5 100644 --- a/snuba/schemas.py +++ b/snuba/schemas.py @@ -291,8 +291,7 @@ def __init__(self, query_schema: Schema, extensions_schemas: Mapping[str, Schema self.__composite_schema['required'] = set(self.__composite_schema['required']) def validate(self, value) -> Request: - # XXX: Mutates input value! - validate(value, self.__composite_schema) + value = validate_jsonschema(value, self.__composite_schema) query = {key: value.pop(key) for key in self.__query_schema['properties'].keys() if key in value} @@ -333,7 +332,12 @@ def validate(self, value) -> Request: }) -def validate(value, schema, set_defaults=True): +def validate_jsonschema(value, schema, set_defaults=True): + """ + Validates a value against the provided schema, returning the validated + value if the value conforms to the schema, otherwise raising a + ``jsonschema.ValidationError``. + """ orig = jsonschema.Draft6Validator.VALIDATORS['properties'] def validate_and_default(validator, properties, instance, schema): @@ -347,6 +351,12 @@ def validate_and_default(validator, properties, instance, schema): for error in orig(validator, properties, instance, schema): yield error + # Using schema defaults during validation will cause the input value to be + # mutated, so to be on the safe side we create a deep copy of that value to + # avoid unwanted side effects for the calling function. + if set_defaults: + value = copy.deepcopy(value) + validator_cls = jsonschema.validators.extend( jsonschema.Draft4Validator, {'properties': validate_and_default} @@ -358,6 +368,8 @@ def validate_and_default(validator, properties, instance, schema): format_checker=jsonschema.FormatChecker() ).validate(value, schema) + return value + def generate(schema): """