serialize query responses according to schema types #53
Conversation
| pub aggregates: Option<Option<::std::collections::HashMap<String, crate::Aggregate>>>, | ||
| pub aggregates: Option<::std::collections::HashMap<String, crate::Aggregate>>, |
There was a problem hiding this comment.
Flattening these double-option wrappings was not strictly necessary. But the double wrapping doesn't do anything for us, and I got tired of dealing with it.
| rating: 7.1 | ||
| rating: | ||
| $numberDouble: "7.1" |
There was a problem hiding this comment.
The strict Extended JSON serialization for rating is expected. In the sample database ratings are a mixture of double and int values. Because we have two different types in the same position type unification resolves the type to ExtendedJSON
| @@ -1269,222 +1261,4 @@ mod tests { | |||
| assert_eq!(v2_request, expected); | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn make_scalar_types() -> BTreeMap<String, ScalarType> { | |||
There was a problem hiding this comment.
I moved these into a shared test_helpers module
|
Ready to review!
Aggregates are still serialized to extjson, and aggregates from relations aren't serialized. I'd like to fix that soon, but we don't need aggregates to work until they're supported by the engine. Edit: I did that hoisting, and I filed https://hasurahq.atlassian.net/browse/MDB-130 to follow up on aggregate serialization |
This is follow up work? Or you did this in this PR? |
| @@ -95,6 +95,30 @@ impl From<Type> for ndc_models::Type { | |||
| } | |||
| } | |||
|
|
|||
| // Should only be used for testing | |||
There was a problem hiding this comment.
Do we have any way of ensuring this for all these?
| @@ -105,6 +105,7 @@ fn find_object_field<'a>( | |||
| ConversionError::UnknownObjectTypeField { | |||
| object_type: object_type.name.to_string(), | |||
| field_name: field_name.to_string(), | |||
| path: Default::default(), // TODO: set a path for more helpful error reporting | |||
There was a problem hiding this comment.
Is this TODO for later or missed in this PR?
There was a problem hiding this comment.
Saving this for later
| |(key, _aggregate_definition)| match aggregate_values.remove_entry(key) { | ||
| Some((owned_key, value)) => Ok(( | ||
| owned_key, | ||
| // TODO: bson_to_json |
There was a problem hiding this comment.
Also later TODO or for this PR?
There was a problem hiding this comment.
Filed https://hasurahq.atlassian.net/browse/MDB-130 to follow up on this
Describe your changes
This changes response serialization to use simple JSON instead of Extended JSON (except in cases of fields with the type
ExtendedJSON).Mutation responses have not been updated with this change yet - that will be in a follow-up PR.
This could be greatly simplified if we had an internal query request type that used copies of object type definitions in all places instead of object type names. I've got some thoughts on changing
v3_to_v2_query_requestto work with an internal type like that instead of with the v2 query request type. But that's a big change that will have to wait until later.Issue ticket number and link
MDB-96