Fixed a bug in DynamicRelation.members() that broke BareAtlas.relationsLowerOrderFirst() #216
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
matthieun
approved these changes
Sep 13, 2018
MikeGost
reviewed
Sep 13, 2018
| Assert.assertEquals(14, finalAtlas.numberOfRelations()); | ||
|
|
||
| /* | ||
| * This has been updated to 16 instead of 14. This is because two of the relations in |
There was a problem hiding this comment.
Not sure this comment is necessary. The check here was on the data, simply changing the number is fine. Thanks for the detailed explanation!
There was a problem hiding this comment.
I honestly left it just for the purposes of explaining the change in the PR, so you guys didn't think I was just cheating to make a test pass :)
It could have been removed but we merged so fast that I didn't get a chance.
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.
Description:
Previously, the interaction between
BareAtlas,AtlasEntity, andDynamicAtlas/DynamicRelationwas broken in the following way:If you had a
DynamicAtlascontaining some relationRthat contained at least one other relationR'as a member, callingDynamicAtlas.relationsLowerOrderFirst()(which used the implementation inBareAtlas) would drop relationRfrom the returned set.This was due to a faulty
AtlasEntityequality assumption made byrelationsLowerOrderFirst()which did not hold in the case ofDynamicAtlas.This has now been fixed in
DynamicRelation. Themembersfunction now returns entities of the correct type, so that the equality assumptions made inrelationsLowerOrderFirst()are correct.Potential Impact:
The broken version of
DynamicAtlas.members()(andBareAtlas.relationsLowerOrderFirst()) was negatively impacting the functionality ofWaySectionProcessor(the raw atlas version), which depends onDynamicAtlasand proper functionality of this method to work. Before the bug fix, theWaySectionProcessorwould drop any relation which had at least one relation as a member.Unit Test Approach:
I have added
DynamicAtlasTest.testRelationsLowerOrderFirstConsistency(), which demonstrates the case in which the bug would occur. It utilizes a new test atlas,DynamicAtlasTestRule.atlasForRelationsTest, which is designed to trigger the bug. With the fix, this unit test now passes.RawAtlasIntegrationTest.testSectioningFromShard()was also affected. In the previous code version, it was dropping two relations from the build, and the test was assuming that those two relations did not exist (when in reality they had actually just been dropped by theWaySectionProcessor). The test is updated, and passes with the bug fix in place.Test Results:
I am planning on doing more extensive testing. Please reach out to me to discuss.
In doubt: Contributing Guidelines