-
Notifications
You must be signed in to change notification settings - Fork 3
Support avro-schema references for records #117
Conversation
1db1c02
to
ab7ca92
Compare
ab7ca92
to
cf34fb6
Compare
graphql/tarantool_graphql.lua
Outdated
@@ -949,6 +970,8 @@ gql_type = function(state, avro_schema, collection, collection_name, field_name) | |||
avro_schema.name, | |||
fields = fields, | |||
}) | |||
state.declarations[avro_schema.name] = types.nonNull(res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand we do not check anywhere if names of named types in our schema are unique (in context of type defining, not referencing) Avro-schema spec states: "A schema or protocol may not contain multiple definitions of a fullname" Moreover on line 973 we may have over writing, haven't we? As in our typical case we have quite massive schemas, unconscious type re-definition may be a problem. So my idea to add some assertion for type re-defenition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitions, sorry. Will rename. Assert seems to make sense for proper error message, will add.
graphql/tarantool_graphql.lua
Outdated
return state.declarations[avro_schema] | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand right that order of definitions in our avro-schema matters? As far as I understand now if we try to put type reference BEFORE type definition in our avro-schema we will have 'unrecognized avro-schema type'. Maybe it is worthy to make additional check and return something like 'reference of yet undefined avro-schema type'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order is matters. You cited the standard right above :)
'unrecognized avro-schema type' seems ok for me. I’ll see whether adding additional check will looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We fall into this error in the two cases:
- Forward reference (forbidden by the standard).
- Non-defined type.
I don’t want to traverse all schema to give some-order-better error message. Will leave as is.
cf34fb6
to
08b5762
Compare
@SudoBobo Force-pushed. |
* fix format_process function * print whole reject file when test failed (#102) * print reproduce file when test failed at -j -1 mode (#104) * print reproduce file content in parallel mode (#104) * allow to pass arguments to create_cluster * allow grep_log to not reset search results after tarantool restart * support comments in config files * don't kill default if non-default crash expected (#109) * don't inherit unneeded file descriptors (#117) * list task for hung workers (#107) * add new config param 'show_reproduce_content' (#113)
* fix format_process function * print whole reject file when test failed (#102) * print reproduce file when test failed at -j -1 mode (#104) * print reproduce file content in parallel mode (#104) * allow to pass arguments to create_cluster * allow grep_log to not reset search results after tarantool restart * support comments in config files * don't kill default if non-default crash expected (#109) * don't inherit unneeded file descriptors (#117) * list task for hung workers (#107) * add new config param 'show_reproduce_content' (#113)
Fixes #116.