ref(schemas): First steps towards isolating query and extension request data by tkaemming · Pull Request #430 · getsentry/snuba

tkaemming · 2019-08-21T01:00:12Z

This is a rethinking of #425 that splits out generic query functionality and dataset-specific extensions.

This change introduces the idea of a "request schema" that merges the query schema and any "extensions" schema that a dataset may use, such time series or performance extensions. This preserves backwards compatibility with the existing query schema by requiring that the names of all properties in the extension schemata form a disjoint set, but also lays the groundwork for a second version of the query schema that can validate each subschema independently.

To limit the scope of this change, this is folded back into a body variable containing all properties at the top level of the map after initial validation.

Follow ups to come will include:

replacing the use of body with request,
adding type annotations (via TypedDict) to the request.query attribute,
better encapsulation of extension behavior (moving column_expr, condition_expr etc.)
moving the schema definition to the Dataset classes themselves and out of snuba/schemas.py

fpacifici

I like the approach, a couple of high level questions.

fpacifici · 2019-08-21T17:01:50Z

+        return Request(query, extensions)
+
+
+EVENTS_QUERY_SCHEMA = RequestSchema(GENERIC_QUERY_SCHEMA, {


What's the advantage of storing all the schemas into one big schemas.py file instead of putting the dataset specific logic (like this class) into the events dataset file? I would put the instantiation of RequestSchema into datasets/events.py like we do for the DDL.

Only change minimization. I think this makes sense as an immediate follow up.

fpacifici · 2019-08-21T17:03:45Z

    try:
-        schemas.validate(body, schema)
+        request = schema.validate(body)
    except jsonschema.ValidationError as error:


I would not explicitly catch jsonschema.ValidationError anymore. In the previous implementation the only method in the try/except block was a direct call to jsonschema validate, thus it was pertinent to expect a jsonschema.ValidationError. Now the type of validation performed by the RequestSchema object is opaque to this method. I would either catch any exception or define a custom validation exception.

I think this makes sense in general.

It's a bit tricky because the control flow for exceptions changed a bit while you were out with #420. The error payload here is dependent on this being a JSON schema, which I think is appropriate. I'll put some thought into figuring out how this should work. Here's where the error is introspected and relevant payload is generated now:

snuba/snuba/api.py

Lines 77 to 83 in 14ad5d8

elif isinstance(cause, jsonschema.ValidationError):

data = {'error': {

'type': 'schema',

'message': cause.message,

'path': list(cause.path),

'schema': cause.schema,

}}

damn, well ok then. I would advise creating a dedicated exception that may wrap the jsonschema.ValidationError but that's not needed in this PR

fpacifici · 2019-08-21T17:07:05Z


    timer.mark('validate_schema')

+    return {**request.body}


I assume you are creating a new dictionary here in order to have a copy. Is that correct? If yes, I think we should think about a plan for mutations of the body and what we want to allow. My opinion is just returning a frozenset or any immutable mapping and completely disallow mutations of the body of the request. I am suggesting this because we are giving more structure to this code than it was used to have, thus keeping track of mutations to a shared daat structure becomes harder.

I assume you are creating a new dictionary here in order to have a copy. Is that correct?

Yeah, this is just for change minimization with the existing code.

If yes, I think we should think about a plan for mutations of the body and what we want to allow. My opinion is just returning a frozenset or any immutable mapping and completely disallow mutations of the body of the request. I am suggesting this because we are giving more structure to this code than it was used to have, thus keeping track of mutations to a shared daat structure becomes harder.

I generally agree your viewpoint here. I think typing will help here since we can enforce this by using Mapping rather than MutableMapping. I don't think we should try and address this as part of this change though, but keep it in mind moving forward.

mind filing a task to rely on immutable data if you do not plan to address the problem right away ?

fpacifici · 2019-08-21T17:12:52Z

+        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():


Here we are building subsets of the original value to validate according to the structure of the subschema. Implicitly additionalProperties would be ignored. Why do we need to set additionalProperties = False in the schema ?

Because right now everything is extracted from a single mapping, it's ambiguous which extension would receive the additional properties as part of its data in the request.extensions map. I don't think discarding them is the right answer either, and disallowing them entirely is just a simpler way of managing that complexity for now. I think we can make it more lax in the future if we move to a more structured query schema similar to the one discussed above.

fpacifici · 2019-08-21T17:16:33Z

+        # 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}


What about we supported a query schema like this?

{ main_field1: value, main_field2: value, timeseries_extension: { time_series_value1: .... ... }, performance_extension: { .... .... } }

While this is not backward compatible, thus we would have to support both formats for a while, this format would have two advantages:

semantics: we would not even have to impose the fields defined by extensions are disjoint. That would be structurally enforced.

The json validation of the extensions would be more sound, since we could just take the extension object and validate it against its extension schema instead of building the composite schema on the fly.

The drawback is that we would commit ourselves to disjoint fields. Which I am not sure it is a bad idea, I cannot think about a strong case for allowing an extension to override a field defined by another one (in terms of validation) that would justify the added complexity of defining the override semantics.

Yes, this is my longer term plan, though I would prefer to structure it as:

{ "query": {…}, # uses generic schema "extensions": { "name": {…}, # uses an extension schema "name": {…} # uses another extension schema } }

(Note that this is intentionally reflective of the structure of the Request dataclass.)

semantics: we would not even have to impose the fields defined by extensions are disjoint. That would be structurally enforced.

I agree with this.

The json validation of the extensions would be more sound, since we could just take the extension object and validate it against its extension schema instead of building the composite schema on the fly.

I think we'll still want the composite schema (as a concept that validates all subschemas in one pass — not this specific implementation that merges them into top level properties), however, as it makes the validation error easier to understand since it's more reflective of the input in totality instead of independently validating each chunk.

The drawback is that we would commit ourselves to disjoint fields. Which I am not sure it is a bad idea, I cannot think about a strong case for allowing an extension to override a field defined by another one (in terms of validation) that would justify the added complexity of defining the override semantics.

I think disjoint fields are only an artifact of the current API/schema implementation, as soon as we move to a new API/schema implementation and remove the old one, we can remove that requirement since it no longer has any benefit.

In my opinion extensions should be considered totally independent — when they are used in query evaluation, they should only get the generic query data as well as their extension parameters, and not have any visibility in to parameters provided to other extensions. (It'll take a little while to get there but that's at least how I think we should move forward.)

fpacifici

could you please address the new comments before committing? they are minor

fpacifici · 2019-08-21T21:28:14Z

    try:
-        schemas.validate(body, schema)
+        request = schema.validate(body)
    except jsonschema.ValidationError as error:


damn, well ok then. I would advise creating a dedicated exception that may wrap the jsonschema.ValidationError but that's not needed in this PR

fpacifici · 2019-08-21T21:28:54Z


    timer.mark('validate_schema')

+    return {**request.body}


mind filing a task to rely on immutable data if you do not plan to address the problem right away ?

fpacifici · 2019-08-21T21:35:12Z

+        # 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}


fpacifici · 2019-08-21T21:41:17Z

+
+
+@dataclass
+class Request:


frozen? (provided it allows you to add an additional field in post_init (I never tried)

This will throw FrozenInstanceError but I can make body a property instead, the ChainMap should be pretty lightweight (compared to a dictionary copy) and is only used once right now anyway.

Did that with f85b70e.

fpacifici · 2019-08-21T21:42:12Z

+
+
+class RequestSchema:
+    def __init__(self, query_schema, extensions_schemas: Mapping[str, Any]):


isn't query_schema supposed to be of type Mapping[str, Any] as well ?

It'd probably be a new Schema type, which would be aliased to … something? Probably some kind of TypedDict? This type annotation would then become:

def __init__(self, query_schema: Schema, extensions_schemas: Mapping[str, Schema]):

Added some minimal stubs with 188cbac, we can expand the type definition later if needed.

fpacifici · 2019-08-21T21:47:28Z

+        self.__composite_schema['required'] = set(self.__composite_schema['required'])
+
+    def validate(self, value) -> Request:
+        # XXX: Mutates input value!


Is it worth making a copy just to stay on the safe side here?
None is supposed to use value after this method anyway so if we throw that result away it is not a an issue. On the other hand we make things safer for the call site.

Sure, I'll do that.

I moved the copy down to validate (and renamed it validate_jsonschema to be more specific) in 70b6e8b, this seemed like a more appropriate place to actually make that change than here.

tkaemming added 2 commits August 20, 2019 17:41

subschemas

71f922c

tidy

6aec0f3

tkaemming requested a review from a team August 21, 2019 01:00

fpacifici reviewed Aug 21, 2019

View reviewed changes

fpacifici approved these changes Aug 21, 2019

View reviewed changes

tkaemming added 4 commits August 21, 2019 15:06

freeze Request

f85b70e

add type stubs for schema

188cbac

don't mutate input value to schema validation

70b6e8b

Merge branch 'master' into query-schemas

c079c59

tkaemming merged commit 9d05a04 into master Aug 21, 2019

tkaemming deleted the query-schemas branch August 21, 2019 22:25

		return Request(query, extensions)


		EVENTS_QUERY_SCHEMA = RequestSchema(GENERIC_QUERY_SCHEMA, {

	elif isinstance(cause, jsonschema.ValidationError):
	data = {'error': {
	'type': 'schema',
	'message': cause.message,
	'path': list(cause.path),
	'schema': cause.schema,
	}}



		class RequestSchema:
		def __init__(self, query_schema, extensions_schemas: Mapping[str, Any]):

Uh oh!

Uh oh!

Conversation

tkaemming commented Aug 21, 2019

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkaemming Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tkaemming Aug 21, 2019 •

edited

Loading