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

Remove BaseModel since it breaks equals/hashCode contract #112

Merged
merged 21 commits into from
Feb 9, 2024

Conversation

Weltraumschaf
Copy link
Member

Problem of equals and inheritance:
As described in this[1] blog post the Object#equals() requires that it fulfills the Liskov Substitution Principle (LSP). Our implemenation breaks this contract. Even worse, it didn't work at all as descibed in issue 23[2].

Since the BaseModel class does not have any properties, we can remove it.

This patch removes this obsolete class. Also it makes all model classes final and all properties private. This is an implicit requirement of the contract of hashCode() to avoid memory leaks in collections. (See linked blog post on Artima for more details.)

Actually objects must be immutable -- all fields final -- to guaruantee a stable equal/hashCode behaviour for the whole lifetime of the objects. But this must be further investigated, if this is possible. For now we ignore this warning in the test code.

1: https://www.artima.com/articles/how-to-write-an-equality-method-in-java
2: #23

@Weltraumschaf Weltraumschaf self-assigned this Jan 26, 2024
@Weltraumschaf Weltraumschaf added the bug Something isn't working label Jan 26, 2024
@Weltraumschaf Weltraumschaf marked this pull request as draft January 26, 2024 19:05
@Weltraumschaf Weltraumschaf marked this pull request as ready for review January 26, 2024 21:44
@Weltraumschaf
Copy link
Member Author

I suggest reviewing by commits instead of files bc I did a reformatting of all classes in one commit.

J12934
J12934 previously approved these changes Jan 30, 2024
Copy link
Member

@J12934 J12934 left a comment

Choose a reason for hiding this comment

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

looks generally good added a couple of comments here to todo question in case that helps give some context

Copy link

sonarqubecloud bot commented Feb 2, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

…tract

Problem of equals and inheritance:
As described in this[1] blog post the Object#equals() requires that it
fulfills the Liskov Substitution Principle (LSP). Our implemenation
breaks this contract. Even worse, it didn't work at all as descibed
in issue 23[2].

Since the BaseModel class does not have any properties, we can remove it.

This patch removes this obsolete class. Also it makes all model classes
final and all properties private. This is an implicit requirement of the
contract of hashCode() to avoid memory leaks in collections. (See linked
blog post on Artima for more details.)

Actually objects must be immutable -- all fields final -- to guaruantee
a stable equal/hashCode behaviour for the whole lifetime of the objects.
But this must be further investigated, if this is possible. For now we
ignore this warning in the test code.

1: https://www.artima.com/articles/how-to-write-an-equality-method-in-java
2: secureCodeBox#23

Signed-off-by: Sven Strittmatter <[email protected]>
Native types should be favored over boxed types for two reasons:

1. Boxed types introduce more verbose code.
2. Boxed types introduce possible NPE.

Since native types reduces code clutter because they have a default value
and they also can not be null, we switched to native type fields.

Signed-off-by: Sven Strittmatter <[email protected]>
Signed-off-by: Sven Strittmatter <[email protected]>
Also add missing null check in implementation.

Signed-off-by: Sven Strittmatter <[email protected]>
Since the implementation of isNameEqual returns false, if the
value in the map is null, we do this for id the same way to be
consistent.

Signed-off-by: Sven Strittmatter <[email protected]>
This is necessary to avoid that something else than types
implementing Model can be wrapped because the service already
require this by upper bounds.

Signed-off-by: Sven Strittmatter <[email protected]>
- Simplify by removing unnecessary setup method.
- Use distinct values in fixture to make it easier spotting bugs.

Signed-off-by: Sven Strittmatter <[email protected]>
Signed-off-by: Sven Strittmatter <[email protected]>
Signed-off-by: Sven Strittmatter <[email protected]>
@J12934
Copy link
Member

J12934 commented Feb 6, 2024

which commits here are new since the last review?
don't want to go through all of them again and with the force pushes & rebases on this branch the dates are wrong and the github ui doesn't show me where to start again.

@Weltraumschaf Weltraumschaf requested a review from J12934 February 6, 2024 15:35
@Weltraumschaf Weltraumschaf merged commit 74aa334 into secureCodeBox:main Feb 9, 2024
2 checks passed
@Weltraumschaf Weltraumschaf deleted the 23_equals branch February 9, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants