Skip to content

Conversation

@paulfrmbstn
Copy link

…sed with combined schema dependencies

@coveralls
Copy link

coveralls commented Feb 26, 2021

Coverage Status

Coverage increased (+0.01%) to 91.559% when pulling 0f32f90 on paulfrmbstn:issue/404 into 23cad68 on everit-org:master.

Copy link
Contributor

@erosb erosb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reporting the issue and raising the PR. It is a bit hacky solution and similar problems can arise later, but if it works for you I'm fine with it. I added a change request.

.filter(schema -> !(schema instanceof CombinedSchema))
.collect(Collectors.toList());

combined.addAll(others);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand here you add the others to a list that was returned by Collectors.toList(). The problem with that is, there is no guarantee for the mutability of this list. As the javadoc says:

There are no guarantees on the type, mutability, serializability, or thread-safety of the List returned;

So please replace this method implementation with any of the followings:

  • a plain old for loop and collecting into two separate ArrayLists
  • Collectors#toCollection(Supplier) so that you can make sure your stream collects into a mutable list
  • or you can have only one stream and sort it based on a comparator sorting by an instanceof CombinedSchema expression

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR with requested changes and added more test coverage. Took the comparator approach.

@erosb
Copy link
Contributor

erosb commented Mar 3, 2021

This looks good now, thank you for addressing my comments.

@erosb erosb merged commit eae88ed into everit-org:master Mar 3, 2021
erosb pushed a commit that referenced this pull request Feb 6, 2025
Maintain insertion order for subschemas of CombinedSchema.

This is an alternative to #519 that maintains the subschemas in insertion order.

The subschemas were originally maintained in insertion order, but it was changed due to #405, which ensured that subschemas that were of type CombinedSchema were visited first during validation, and by #498, which tries to maintain a stable order for "equivalent" instances.

This PR simply restores the insertion order, but maintains an internally sorted collection for the two purposes listed above, namely visiting subschemas during validation and equivalence checks during equals/hashCode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants