Updates to annotation validation and validation test additions#3450
Open
drewfarris wants to merge 6 commits into
Open
Updates to annotation validation and validation test additions#3450drewfarris wants to merge 6 commits into
drewfarris wants to merge 6 commits into
Conversation
Collaborator
drewfarris
commented
Mar 3, 2026
drewfarris
commented
- Now provides separate validators for checking annotation data vs. annotation identity, so that writes of unidentified annotations can be performend and validated prior to assigning ids.
- Updates ingest tests to add segment hashes
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
2 times, most recently
from
2d579d7 to
34b3d92
Compare
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
2 times, most recently
from
81c36e2 to
539f419
Compare
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
from
539f419 to
f95a2f0
Compare
drewfarris
changed the title
FineAndDandy
requested changes
Mar 31, 2026
Collaborator
FineAndDandy
left a comment
FineAndDandy
left a comment
There was a problem hiding this comment.
a few questions otherwise looks good
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
from
f95a2f0 to
94790a3
Compare
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
2 times, most recently
from
ee1537b to
23c8720
Compare
Comment on lines
+404
to
+408
| // If we get here, the delete handled empty results correctly | ||
| } catch (IllegalArgumentException e) { | ||
| // This is expected if mutationAdapter returns null and writer.addMutation is called with null | ||
| // The implementation should be fixed to check for null mutations | ||
| assertTrue(e.getMessage().contains("m is null")); |
Collaborator
There was a problem hiding this comment.
which case are you expecting here? The silent pass through or the catch on the IllegalArgumentException?
Collaborator
Author
There was a problem hiding this comment.
Thanks for catching this. I meant to modify the logic to throw an exception when the object to delete is not found - matching the semantics for an update operation.
FineAndDandy
previously approved these changes
Apr 22, 2026
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
2 times, most recently
from
93ffedd to
85d12ee
Compare
avgAGB
reviewed
May 1, 2026
| if (blockAnnotationOverwrites) { | ||
| throw new AnnotationWriteException("Cannot add annotation because an annotation with the same id already exists."); | ||
| } else { | ||
| log.debug("Allowing annotation overwrite: {}", annotationIdContext(conflicting.get())); |
Collaborator
There was a problem hiding this comment.
similar note here if applicable
tharke13
reviewed
May 11, 2026
* Now provides separate validators for checking annotation data vs. annotation identity, so that writes of unidentified annotations can be performend and validated prior to assigning ids. * Updates ingest tests to add segment hashes
* Demonstrates that empty source is provided if no source is specified in the Annotation builder. * Demonstrates that the builder won't accept null and thus the validations don't need to explicitly check for null.
drewfarris
force-pushed
the
feature/annotation-validation-update
branch
from
85d12ee to
b274cb8
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.