-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Introspect Schema endpoint #6731
Conversation
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
🦋 Changeset detectedLatest commit: 0e4bd77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
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.
This looks good to me. I wonder if we shouldn't merge this into release-5 though? Would also like Brian to look at it.
@sujithvn Because this requires a mandatory migration I am changing the root branch to |
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.
There are a lot of additional changes in the diff due to pointing at the release branch. Those changes should be merged first or this PR should be rebased off the release branch.
I did my best to review the introspection files, though. See my inline comments.
packages/api-plugin-simple-schema/src/queries/introspectSchema.js
Outdated
Show resolved
Hide resolved
packages/api-plugin-simple-schema/src/queries/introspectSchema.js
Outdated
Show resolved
Hide resolved
packages/api-plugin-simple-schema/src/queries/introspectSchema.js
Outdated
Show resolved
Hide resolved
… release-5 Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
Signed-off-by: Sujith <mail.sujithvn@gmail.com>
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.
I didn't actually test it, but from a code review perspective this LGTM now!
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.
Works on my local.
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.
Excited to see this finally merged
Signed-off-by: Sujith mail.sujithvn@gmail.com
Resolves #6529 #6438
Impact: major
Type: feature
Issue
There is a requirement to introspect any given schema to analyse the fields available and its type with proper permission validation.
Solution
We have developed a new query end-point [introspectSchema] which accepts the schemaName and shopId and returns the schema details as a JSONObject.
Breaking changes
All the changes are new and does not impact any existing setup. But since this change will need additional permissions, it will have a mandatory migration to be done to start using this endpoint.
Testing
Unit test cases currently failing due to issues of the latest simpleSchema 3.4.0 with Jest.