-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix: Incorporate schema root into docID #2701
fix: Incorporate schema root into docID #2701
Conversation
Request: fmt.Sprintf(`mutation { | ||
delete_Book(docID: "%s") { | ||
Request: `mutation { | ||
delete_Book(docID: "bae-b5c56d8f-b2f5-57f9-b371-4e9e04903e91") { |
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.
question: Is it necessary to hardcode the docID here? You're doing the effort to switch to NewDocIndex
in a lot of places. It seems like these would also be good candidates for that.
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 do not have support for DocIndex
within the body of requests yet. This PR makes use of DocIndex
where it (in it's current state) made the update easier, but the goal of this PR is not to propagate the usage of DocIndex
, or to improve it.
This PR makes a needed production change to the DocID generation, and then updated the tests in the easiest way available.
b49e9dd
to
bd1eee0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2701 +/- ##
===========================================
+ Coverage 77.93% 78.01% +0.08%
===========================================
Files 308 308
Lines 23134 23135 +1
===========================================
+ Hits 18028 18048 +20
+ Misses 3721 3708 -13
+ Partials 1385 1379 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
LGTM!
ed1893c
to
c8378a2
Compare
c8378a2
to
bb230fe
Compare
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.
LGTM must have been a pain to update all these, cheers!
Glanced over / skipped reviewing the ID diffs but rest looks good.
Relevant issue(s)
Resolves #2688
Description
Incorporates schema root into docID.
The production changes are all done in the first two commits, I broke out the test changes to a separate commit to avoid confusion. There are no significant test changes - is just really really painful updating this as besides the docIDs and cids, the order in which results were returned changed.