-
Notifications
You must be signed in to change notification settings - Fork 3k
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(siblings) Fix editing of schema fields for siblings with unequal schemas #7199
Merged
chriscollins3456
merged 5 commits into
datahub-project:master
from
chriscollins3456:cc--edit-schema-for-siblings
Feb 2, 2023
Merged
fix(siblings) Fix editing of schema fields for siblings with unequal schemas #7199
chriscollins3456
merged 5 commits into
datahub-project:master
from
chriscollins3456:cc--edit-schema-for-siblings
Feb 2, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chriscollins3456
requested review from
gabe-lyons,
jjoyce0510 and
aditya-radhakrishnan
January 31, 2023 18:39
gabe-lyons
approved these changes
Jan 31, 2023
jjoyce0510
reviewed
Jan 31, 2023
...l-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTagsResolver.java
Show resolved
Hide resolved
jjoyce0510
reviewed
Jan 31, 2023
...-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTermsResolver.java
Show resolved
Hide resolved
jjoyce0510
reviewed
Jan 31, 2023
@@ -107,6 +115,36 @@ private CompletableFuture<Boolean> updateDomainDescription(Urn targetUrn, Descri | |||
}); | |||
} | |||
|
|||
// If updating schema field description fails, try again on a sibling until there are no more siblings to try. Then throw if necessary. |
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.
Minor: try to use javadocs format:
/**
- Attempts to update....
*/
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.
(may not be there yet)
jjoyce0510
reviewed
Jan 31, 2023
...hql-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/util/SiblingsUtils.java
Show resolved
Hide resolved
jjoyce0510
reviewed
Jan 31, 2023
...e/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/UpdateDescriptionResolver.java
Outdated
Show resolved
Hide resolved
jjoyce0510
reviewed
Jan 31, 2023
...l-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTagsResolver.java
Show resolved
Hide resolved
jjoyce0510
reviewed
Jan 31, 2023
...-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTermsResolver.java
Show resolved
Hide resolved
jjoyce0510
reviewed
Feb 1, 2023
...l-core/src/main/java/com/linkedin/datahub/graphql/resolvers/mutate/BatchAddTagsResolver.java
Show resolved
Hide resolved
jjoyce0510
approved these changes
Feb 2, 2023
ericyomi
pushed a commit
to ericyomi/datahub
that referenced
this pull request
Feb 8, 2023
oleg-ruban
pushed a commit
to RChygir/datahub
that referenced
this pull request
Feb 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes a bug we were seeing where users were getting errors trying to edit schema fields for an entity with siblings where the two siblings either has mismatched schemas (edit a column that doesn't exist in the primary sibling) or if the primary sibling doesn't have a schema at all.
There were multiple different ways we could go about solving this, but we settled on the simplest way to just find the next sibling if something fails when editing schema fields and try that one. If there are no siblings or we try all siblings and keep failing, then throw the error.
This fix feels a bit hacky, but I think is the least risky path forward right now. It involves updating the 3 resolvers for editing schema field descriptions, terms, and tags.
Also fixes a UI sibling merge bug like we had before for schema fields, but for editable schema fields now. Merge based on key, not on the index of the array so we can display editable schema metadata info between siblings properly.
Checklist