Skip to content
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

#302: do not use ReferenceSchema#referredSchema for equals+hashCode #378

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tmarsteel
Copy link

This solves Issue #302. Not including the referred schema feels right to me since it depends on the schemaLoader. I have no idea whether people rely on referredSchema being in equals+hashCode, though.

@coveralls
Copy link

coveralls commented Jul 17, 2020

Coverage Status

Coverage decreased (-0.04%) to 91.532% when pulling 631d08d on rebuy-de:cyclic-schema into fd33c26 on everit-org:master.

@@ -145,7 +145,6 @@ public boolean equals(Object o) {
return that.canEqual(this) &&
Objects.equals(refValue, that.refValue) &&
Objects.equals(unprocessedProperties, that.unprocessedProperties) &&
Objects.equals(referredSchema, that.referredSchema) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @tmarsteel , do you think we loose anything if we keep this line? So my concern is that, with this change, there can be (rare) cases when two schemas are considered equal when they are not (I mean eg. two references pointing to # , but this denotes the roots of two different schema documents).
What do you think about keeping this line? I see that it might run into infinite equals() recursions, but that's probably a very rare case.

Choose a reason for hiding this comment

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

In the case of Confluent Schema Registry, we call equals quite often on schema. So we would prefer the fix above.

Copy link
Author

Choose a reason for hiding this comment

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

That's a very good point. The testcase and my actual cyclic schemas both work with referredSchema being included in equals.

@tmarsteel tmarsteel requested a review from erosb July 24, 2020 11:09
@tmarsteel
Copy link
Author

Hm, this doesn't suffice yet. The HashCodeRecursionTest succeeds in my IDE but fails in maven. Looking into it.

@tmarsteel
Copy link
Author

tmarsteel commented Jul 24, 2020

Okay, so it turns out that equals will always cause a stackoverflow when you call it on equal (but not identical) schemas. I have such a cycle in one of my projects, so keeping referredSchema in equals doesn't work for me. I see two solutions for me here :D

  1. modify my own code to assure any schema file is not loaded more than once so the equals method can take the identity-shortcut
  2. Since ReferenceSchema is the only way to create cycles in json schemas, these cycles can be detected and handled there. I have drafted something (13438c9) that passes the tests, but i'm very unsure whether this code should go productive anywhere

@rayokota
Copy link

rayokota commented Jul 24, 2020

@tmarsteel @erosb We would prefer the fix where referredSchema is excluded from both equals and hashCode.

If a client wants to additionally compare the referredSchema, the client can always do that check "manually". Optionally perhaps a deepEquals method can be provided that does that. However, without this PR, there is no way to avoid the StackOverFlowError.

In other words, we need one method to avoid the StackOverFlowError. Optionally, you can provide a separate method deepEquals, which might still result in the error.

@tmarsteel
Copy link
Author

@rayokota that sounds very reasonable to me. I'm okay with implementing it. It would be a breaking change, so likely the release will not be that soon, will it? @erosb

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.

None yet

4 participants