eclipse-leshan / leshan

Java Library for LWM2M
https://www.eclipse.org/leshan/
BSD 3-Clause "New" or "Revised" License
653 stars 408 forks source link

Equals&hashCode fixup #1644

Closed nkrzykala closed 2 months ago

nkrzykala commented 3 months ago

This pull request is according to #1504

I separated modified and added files into commits by categories. You can find commits dividing:

sbernard31 commented 2 months ago

I hope you will not be too afraid with all those comments ! Not the best way to welcome back :grin:

The idea to separate PR in several commits was good but with hindsight maybe we should do that more step by step. Maybe we several small PR with just some classes :

Reading you code, I also ask myself how Attribute class hierarchy should be compared :thinking: but this is out of scope of this PR.

sbernard31 commented 2 months ago

Reading Objects.hash(Arrays.hashcode()), I see that HkdfAlgorithm has no hashcode/equals method but is used in OscoreSetting which could lead to expected result :thinking: ?

nkrzykala commented 2 months ago

Could you please explain https://github.com/eclipse-leshan/leshan/pull/1644#issuecomment-2324337706, I don't understand what you mean? That we don't know how HkdfAlgorithm will be hashed because it doesn't define equals/hashcode?

sbernard31 commented 2 months ago

Could you please explain https://github.com/eclipse-leshan/leshan/pull/1644#issuecomment-2324337706, I don't understand what you mean? That we don't know how HkdfAlgorithm will be hashed because it doesn't define equals/hashcode?

I added this comment just to remember that I just see that HkdfAlgorithm doesn't have custom equals/hashcode and I think we don't want to keep the default behavior.

nkrzykala commented 2 months ago

Oh, OK sorry for misunderstanding 😄

nkrzykala commented 2 months ago

All mentioned changes are implemented. I know that, as you mentioned in https://github.com/eclipse-leshan/leshan/pull/1644#issuecomment-2310532778, I should have created separate PRs... should I do that now or stick with this PR?

sbernard31 commented 2 months ago

I should have created separate PRs

My fault too, I should advice you to do that.

should I do that now or stick with this PR?

As you prefer. As I said small PR you will have better and faster feedback. But now you have the whole code done, this will maybe be painful to split it.

sbernard31 commented 2 months ago

@nkrzykala is this ready for review ?

nkrzykala commented 2 months ago

@nkrzykala is this ready for review ?

Yes, sorry I didn't let you know

nkrzykala commented 2 months ago

Fixed those 3 issues

nkrzykala commented 2 months ago

Hi, I forgot to do rebase of my branch onto master before I pushed last commits - now my branch is behind by 4 commits:

I am not sure how to fix that as now locally I have the right version (those 4 commits + my commits, after rebase) and remotely I have all of my commits and not those 4 ones. I have question if you could help me clean that?

sbernard31 commented 2 months ago

Hi, I forgot to do rebase of my branch onto master before I pushed last commits - now my branch is behind by 4 commits:

Not a big deal, if there is not so much conflict I can do that on myself.

I am not sure how to fix that as now locally I have the right version (those 4 commits + my commits, after rebase) and remotely I have all of my commits and not those 4 ones. I have question if you could help me clean that?

So generally, I create another local branch for the current remote. (so If I do something wrong and can easily go back to that state) Then once I did all my history rewriting locally and I have my local branch in the wanted state, I just push -force the branch.

Let me know if you want to try to do that OR if I do it myself when I will integrate it in master ?

(I fetched your current remote code on my machine, so if something goes wrong I have a backup :wink:)

nkrzykala commented 2 months ago

If it wouldn't be much work and a problem - I'd prefere you to take care of it. I don't think I trust myself with that 😄 Thank you very much!

nkrzykala commented 2 months ago

Hi, while looking at possible solutions for another issue (https://github.com/eclipse-leshan/leshan/issues/1279) I found out that there is an old implementation of EqualsVerifiers test for Version class in VersionTest.java. I missed to delete it because my implementation of this test is in Lwm2mTest.

sbernard31 commented 2 months ago

I found out that there is an old implementation of EqualsVerifiers test for Version class in VersionTest.java. I missed to delete it because my implementation of this test is in Lwm2mTest.

Thx for letting me know that. I will look at that.

sbernard31 commented 2 months ago

So,

I integrated this PR in master (commit : 70eec7dee8170dce907b5cbc712171469ede1e8d)

About importing junit5 "correctly", this is fixed by 629352345b65571843745f4e7019d1e20f3a3297)

About adding virtualHost to IpPeer equals/hashcode(), this is fixed by 47890ddd8946c5489bc7395a8817300e59283747

About VersionTest vs LwM2mTest, this is fixed by : ccd7bc91eb0ae2a37d86d3db4d329f1dd4a3afaf

So I think we can close this issue.

Thx a lot @nkrzykala for your contribution ! Really Good Job ! I really appreciate it :pray: