Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Benoit/eng-362-update-ndc-postgres-to-ndc_models-020 #666
base: main
Are you sure you want to change the base?
Benoit/eng-362-update-ndc-postgres-to-ndc_models-020 #666
Changes from 41 commits
c1a0017
f4c4495
91e9047
d206136
25b96a7
96985b5
8f5f678
0be15c4
47245b8
3131257
7277f34
8dc0211
16e32fe
a87ffc9
1a1c0ce
08612c1
3740a0e
4b2d373
3694e96
9c4c662
6ea5b3a
e947d61
bea5ab4
bc4138b
67595f0
0598a2f
14cad36
cf47f9c
a874165
0bfd425
46f4ab9
a77fbf6
d56fb32
7ed4cb6
80f187e
dc70526
cb3cce7
3102706
67d8de6
829886f
15fa6df
64b6ef9
eed2de8
c51323e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Can we bump this separately before the PR goes in? Good not to mix up functional and non-functional changes.
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.
No.
ndc-test
needs to be updated alongsidendc-models
andndc-sdk
, and it usesreqwest
12
, which is the reason for this change.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.
V0.2.0 requires type representation, but configuration did not require configuration to be present.
To maximize compatibility with older configuration versions, we infer missing type representations based on scalar type name. If missing, we default to JSON representation.
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 already have the
default_base_type_representations
to do this mapping from scalar type name toTypeRepresentation
. If this function adds any more mappings we should add them there rather than adding a new layer of indirection. The idea that whatever mappings are in the configuration are what will be returned in the schema is a useful property for understanding all this, and we would need a compelling reason to remove that property.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.
v0.2 requires type representation in schema responses, however type representation is optional in the configuration itself.
So this is a piece of backwards compatiblity code, to allow for configurations that are missing type representation.
We could instead create configuration v6, and refuse to run with older configuration versions.
As for the use of
default_base_type_representations
: this mapping only exists for configurations v4 and v5. Here in v3 we replicate that functionality instead.I prefered copying the code but keeping the different modules separate.
And yes it's a bit cheaty: we have a default when creating a new config, and reuse that default when type representation is missing.
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's probably best to leave version 3 configs doing whatever they were before, ie falling back to JSON where they have no
TypeRepresentation
, or where they used the deprecatedNumber
orInteger
types.Also, there is no need for a version 6, it looks like we were already making any mention of
Number
andInteger
fallback to JSON before this PR, and since we have our own type exposed in the config type, we won't be removing any items and thus making any breaking changes.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.
Defaulting to JSON is a behavior change. Representation was optional in both configuration and schema response, so we just returned it as-is.
This applies to configurations 3,4,5
Confirm we'd rather default to JSON if representation missing, without first trying to infer based on type name?
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.
Default introspection configuration changed to tag lt(e),gt(e) operators.
This will only affect new configurations, so any deployments with existing configuration will see no change in behavior.
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.
V0.2.0 requires type representation.
Type representation comes from introspection configuration, and may be absent.
So, if type representation is missing, we infer the type based on the name and fetch the corresponding type representation from the default introspection configuration.