- Notifications
You must be signed in to change notification settings - Fork12
Remove BaseModel since it breaks equals/hashCode contract#112
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I suggest reviewing by commits instead of files bc I did a reformatting of all classes in one commit. |
src/main/java/io/securecodebox/persistence/defectdojo/model/Response.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this 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
src/main/java/io/securecodebox/persistence/defectdojo/model/UserProfile.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
|
…tractProblem of equals and inheritance:As described in this[1] blog post the Object#equals() requires that itfulfills the Liskov Substitution Principle (LSP). Our implemenationbreaks this contract. Even worse, it didn't work at all as descibedin 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 classesfinal and all properties private. This is an implicit requirement of thecontract of hashCode() to avoid memory leaks in collections. (See linkedblog post on Artima for more details.)Actually objects must be immutable -- all fields final -- to guaruanteea stable equal/hashCode behaviour for the whole lifetime of the objects.But this must be further investigated, if this is possible. For now weignore this warning in the test code.1:https://www.artima.com/articles/how-to-write-an-equality-method-in-java2:secureCodeBox#23Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
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 valueand they also can not be null, we switched to native type fields.Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Also add missing null check in implementation.Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
…memtationsSigned-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Since the implementation of isNameEqual returns false, if thevalue in the map is null, we do this for id the same way to beconsistent.Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
This is necessary to avoid that something else than typesimplementing Model can be wrapped because the service alreadyrequire this by upper bounds.Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
- Simplify by removing unnecessary setup method.- Use distinct values in fixture to make it easier spotting bugs.Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
…ic APISigned-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
which commits here are new since the last review? |
src/main/java/io/securecodebox/persistence/defectdojo/model/PaginatedResult.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/main/java/io/securecodebox/persistence/defectdojo/model/PaginatedResult.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/main/java/io/securecodebox/persistence/defectdojo/model/PaginatedResult.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/main/java/io/securecodebox/persistence/defectdojo/model/ScanFile.java OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
…ns HaveSigned-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
Signed-off-by: Sven Strittmatter <sven.strittmatter@iteratec.com>
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