Allowlist metadata Import bug #4762
Conversation
|
Review app for commit 94b828f deployed to Heroku: https://hge-ci-pull-4762.herokuapp.com |
|
Review app for commit 38fa300 deployed to Heroku: https://hge-ci-pull-4762.herokuapp.com |
- it checks if the metadata is the same when created through APIs and while importing the metadata into graphql-engine
|
Deploy preview for hasura-docs ready! Built with commit 16cd83c |
|
Review app for commit 59a0f04 deployed to Heroku: https://hge-ci-pull-4762.herokuapp.com |
server/tests-py/test_v1_queries.py
Outdated
| @@ -535,6 +535,10 @@ def test_export_replace(self, hge_ctx): | |||
| } | |||
| replace_code, replace_resp, _ = hge_ctx.anyq(url, replace_query, headers) | |||
| assert replace_code == 200, replace_resp | |||
| # The export_resp and export_resp_1 should be the same | |||
There was a problem hiding this comment.
It will greatly help to have an explanation on why we are doing this check, the assert export_resp == export_resp_1 already conveys this comment.
There was a problem hiding this comment.
I have added some comments to explain the test.
|
Review app for commit b81801e deployed to Heroku: https://hge-ci-pull-4762.herokuapp.com |
server/tests-py/test_v1_queries.py
Outdated
| # after creating the metadata through the metadata APIs and the metadata | ||
| # exported after importing the metdata using the `replace_metadata` to be | ||
| # the same. Doing this check, ensures that the metadata has been properly | ||
| # imported by the graphql-engine. |
There was a problem hiding this comment.
This is definitely better but if you think about it, this content
we are again exporting the metadata to assert that the metadata exported
after creating the metadata through the metadata APIs and the metadata
exported after importing the metdata using thereplace_metadatato be
the same.
can again be inferred by reading the code easily. What is hard to understand is this part:
Doing this check, ensures that the metadata has been properly
imported by the graphql-engine
I spent a good few minutes trying to understand how this catches the current bug at hand, you are exporting, then importing and then exporting again, why would anything change? The 'why' is the most important part that should go into the comment:
This test catches incorrect key names in
export_metadataserialization, for example ifallowlistis misspelled asallow_list, the first export metadata would have{..., "allow_list":, ...}but when you import this and export the metadata again, this incorrect key will be absent becausereplace_metadatadoesn't import the incorrectly serialized metadata and therefore there is nothing about allowlist that can be exported the second time and hence the two exports will differ.
There was a problem hiding this comment.
I have changed the comment a bit
This test catches incorrect key names(if any) in the
export_metadataserialization, for example, A new query collection is added to the allow list using theadd_collection_to_allowlistmetadata API. When
the metadata is exported it will contain theallowlist. Now, when this
metadata is imported, if the graphql-engine is expecting a different key
likeallow_list(instead ofallowlist) then the allow list won't be imported. Now, exporting the metadata won't contain theallowlistkey
because it wasn't imported properly and hence the two exports will differ.
Let me know, what you think
|
Review app for commit 4d858dc deployed to Heroku: https://hge-ci-pull-4762.herokuapp.com |
|
Review app https://hge-ci-pull-4762.herokuapp.com is deleted |
fix bug which was not allowing the allowlist to be imported
fix bug which was not allowing the allowlist to be imported
fix bug which was not allowing the allowlist to be imported
fix bug which was not allowing the allowlist to be imported
fixes #4687
Description
Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR.Affected components
Related Issues
Solution and Design
Earlier, while importing the metadata, the key expected for allow-list was
allow_listand the key used while exporting isallowlist. Due to this mismatch, the allow list was not getting imported.Solution: Change the key name from
allow_listtoallowlist.Steps to test and verify
Limitations, known bugs & workarounds
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
run_sqlauto manages the new metadata through schema diffing?run_sqlauto manages the definitions of metadata on renaming?export_metadata/replace_metadatasupports the new metadata added?GraphQL
Breaking changes
No Breaking changes
There are breaking changes:
Metadata API
Existing
querytypes:argspayload which is not backward compatibleJSONschemaGraphQL API
Schema Generation:
NamedTypeSchema Resolve:-
nullvalue for any input fieldsLogging
JSONschema has changedtypenames have changed