ndc-spec 0.1.2 #408
ndc-spec 0.1.2 #408
Conversation
| metadata::TypeRepresentation::Integer => models::TypeRepresentation::Int32, | ||
| metadata::TypeRepresentation::Number => models::TypeRepresentation::Float64, |
There was a problem hiding this comment.
Stop gap until we remove these.
|
Dismissing my review because I just realized that the ndc-sdk commit points to a branch. Let's wait until that's merged and update the commit. |
| } | ||
|
|
||
| /// Map our local type representation to ndc-spec type representation. | ||
| fn map_type_representation( |
There was a problem hiding this comment.
I'm not sure we should be changing the outputted type representations like this without the user providing us an opt-in config flag to do so.
There was a problem hiding this comment.
This part is just translating our own TypeRepresentation type to ndc-spec TypeRepresentation type. Could you please elaborate on what do you mean? I'm not sure I understand.
There was a problem hiding this comment.
I was under the impression that our types should return the same ndc-spec TypeRepresentation as they did already before this change. Where this is not possible, because of removed types, we should return the more general json. Otherwise we're potentially changing behaviour without the user opting into it.
There was a problem hiding this comment.
Changing the type representations themselves to what they are supposed to be is not a breaking change, and the previous values (Integer, Number) are deprecated.
What's breaking is if we decide to say that bigints have the representation Int64, because:
- If we provide values for bigints as we currently do, as numbers, it would be against the spec
- If we change the runtime behavior and cast bigints to strings, we will return different results than we do now.
What we do here is to map each type (we know) to its right type representation, except bigints, which we map to json, and this is done in the version3/mod.rs code.
There was a problem hiding this comment.
What will a version of engine that does not know about these types do when it receives them?
There was a problem hiding this comment.
This is an ndc-schema concept that can be used by cli/lsp to generate engine objects, so I don't think this will be used directly in the engine. The engine already uses v0.1.2 of ndc-spec though: https://github.com/hasura/v3-engine/pull/443/files#diff-5e05c14aa4760c15d75bc66970581abe17a82eef4770a0cd9861bd40ce65934bR27
There was a problem hiding this comment.
Ahh OK, makes sense. This all seems sensible then, thank you for indulging my questions.
| // This is not what we do now and is a breaking change. | ||
| // This will need to be changed in the future. In the meantime, we report | ||
| // The type representation to be json. | ||
| database::TypeRepresentation::Json, |
| "float8": "float64", | ||
| "int2": "int16", | ||
| "int4": "int32", | ||
| "int8": "json", |
There was a problem hiding this comment.
These are the changes to the configuration. Number and String are deprecated in ndc-spec. We will still accept these for backwards compatibility reasons, but we will output the new representations. @danieljharvey
| "even_number": { | ||
| "representation": { | ||
| "type": "integer" | ||
| "type": "int32" |
There was a problem hiding this comment.
These are the changes to the ndc schema.
| ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs.git", rev = "7b56fac3aba2bc6533d3163111377fd5fbeb3011" } | ||
| ndc-test = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.1" } | ||
| ndc-models = { git = "https://github.com/hasura/ndc-spec.git", tag = "v0.1.2" } | ||
| ndc-sdk = { git = "https://github.com/hasura/ndc-sdk-rs.git", rev = "7409334" } |
There was a problem hiding this comment.
I would prefer to use the full SHA here because Cargo thinks two dependencies pointing to the same revision but using a different prefix length are actually different dependencies. This causes serious issues when using ndc-postgres as a dependency as it brings in ndc-sdk with the short ref, so everything else needs to use the exact same prefix length.
There was a problem hiding this comment.
Good to know. Will fix.
### What See comment #408 (comment)
3 participants
What
Support ndc-spec v0.1.2.
This includes changing the type representations of some types.
How
The type representation has changed. We try to keep this change backwards compatible by mapping deprecated variants (Number, Integer) to new variants, and update types to more specific reps that didn't exist before.
We also have a problem with representing int8 now. ndc-spec v0.1.2 defines Int64 to be represented as a json string, which is not what we do. So we either have to lie (say it's an Int32 when it isn't) or break compatibility and cast int8 to string.
In this PR I did the former, and I'll introduce the latter change next.