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

V1Secret equals always returns false #431

Open
davidxia opened this issue Nov 9, 2018 · 14 comments
Open

V1Secret equals always returns false #431

davidxia opened this issue Nov 9, 2018 · 14 comments

Comments

@davidxia
Copy link
Contributor

@davidxia davidxia commented Nov 9, 2018

Two V1Secret instances deserialized from the same YAML are not equal. In fact, V1Secret.equals(Object o) always returns false. This is because V1Secret.data = Map<String, byte[]>.

private Map<String, byte[]> data = null;

The equals method on a Java array type is equivalent to ==. So this line returns false.

Objects.equals(this.data, v1Secret.data) &&

Would you be open to patches for this? If so, what's the right approach? Keep the V1Secret.data = Map<String, byte[]> and modify the line above to loop through the map checking keys are equal with Objects.equals() and byte[] values are equal with java.util.Arrays.equals(...)?

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 9, 2018

This is really very unfortunate. That code is generated, so we can't manually patch it. We could override it with our own class (similar to IntToString or Quantity) if we really wanted to.

Alternately, we could update the SwaggerCodegen here:

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/Java/pojo.mustache#L164

To be smarter...

@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Nov 9, 2018

@brendanburns thanks for explaining. Any preference of approach? I can try to make a PR for the first approach if it'll help.

@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Nov 11, 2018

@brendanburns Ah I see what you meant by linking to SwaggerCodegen. Looks like that line to handle byte arrays was fixed by this PR. Does this mean the version of SwaggerCodegen used by this repo is outdated and should be upgraded? Maybe an upgrade of SwaggerCodegen will also fix #340.

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 13, 2018

@davidxia unfortunately, I don't think that PR fixes this issue. The object in question here is a Map and so I think it still calls Objects.equal(...)

Looking at the code, I think the right answer is to use Objects.deepEquals(...) here instead of Objects.equals(...)

https://docs.oracle.com/javase/7/docs/api/java/util/Objects.html#deepEquals(java.lang.Object,%20java.lang.Object)

We should do a PR into swagger-code to update to deepEquals

@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Nov 14, 2018

I took a closer look at this and it seems Object.deepEquals() won't work either. I assume you were thinking about having the deepEquals() here?

Objects.equals(this.data, v1Secret.data) &&

This will go here

return Arrays.deepEquals0(a, b);

which, because a and b are LinkedHashMaps, will reach this with e1 and e2 being the same LinkedHashMaps.

 eq = e1.equals(e2);

false is finally returned from here because value and m.get(key) are the byte[]. This is where deepEquals() would've worked.

if (!value.equals(m.get(key)))
    return false;

So I think using both Objects.equals() and Objects.deepEquals() will end up at that line above. I don't see any straightforward fixes to this right now...

I created a failing test here and an example of how Objects.deepEquals() fails here.

@brendandburns
Copy link
Contributor

@brendandburns brendandburns commented Nov 14, 2018

Wow, that's really unfortunate. It feels like a bug in the Java code, I would have expected deepEquals of two maps with the same keys and the same values to return true...

We could in theory hack up the swagger codegen to detect maps and do the right thing here itself (recursively, of course).

It feels like a lot of work for small payoff, but we can keep the issue open in case someone wants to take it up...

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Apr 26, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@fejta-bot
Copy link

@fejta-bot fejta-bot commented May 27, 2019

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Jun 15, 2019

/remove-lifecycle rotten

@yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Jun 17, 2019

am in favor of an upstream fix before we migrating to openapi-generator #595, and so do the other issues complaining about the template'd codes.

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Sep 17, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@davidxia
Copy link
Contributor Author

@davidxia davidxia commented Sep 17, 2019

/remove-lifecycle stale

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Dec 16, 2019

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@yue9944882
Copy link
Member

@yue9944882 yue9944882 commented Dec 16, 2019

/lifecycle frozen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants