Skip to content

fix: change entity association in UserOptionController - #93

Merged
plozanol merged 6 commits into
mainfrom
fix/change-entity-association
Dec 13, 2022
Merged

fix: change entity association in UserOptionController#93
plozanol merged 6 commits into
mainfrom
fix/change-entity-association

Conversation

@mikededo

@mikededo mikededo commented Dec 11, 2022

Copy link
Copy Markdown
Contributor

Description

  • Fixed the Team-Repository association in the UserOptionController.
  • Fixed the Organization-Team association in the UserOptionController.

Decisions taken

I opted to override the hashCode and equals methods using only the id
field. The reason is because if done properly, we should not have duplicated
entities and each entity is uniquely identified by the id field.

Closes #92

@mikededo mikededo added this to the 2 - MVP milestone Dec 11, 2022
@mikededo mikededo added status: code review PR ready to be reviewed type: bug Something isn't working labels Dec 11, 2022
@mikededo mikededo self-assigned this Dec 11, 2022
@mikededo
mikededo requested a review from a team December 11, 2022 19:00

Organization that = (Organization) o;

return id.equals(that.id);

@plozanol plozanol Dec 11, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Because id is a Integer object, you should first check if it isn't null. Also when overriding default behavior a test should be added to check consistency in this implementation.

If you want happy path and less boiler code, you could use lombok @EqualsAndHashCode annotation:
https://projectlombok.org/features/EqualsAndHashCode

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding this, I get this warning:

Using @EqualsAndHashCode for JPA entities is not recommended. It can cause severe performance and memory consumption issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also, about the null subject, we have the Column annotation with the nullable = false. Furthermore, I think we could ensure more the type-safetiness using the @NotNull annotation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Regarding this, I get this warning:

Using @EqualsAndHashCode for JPA entities is not recommended. It can cause severe performance and memory consumption issues.

Yes, you are right, didn't remember that. Then hardcoded better to avoid performance issues. Go ahead with your implementation, seems good enough. It has some issues with JPA state entity lifecycle, but I think we shouldn't worry right now, as that inconsistencies didn't apply to our app.

https://vladmihalcea.com/how-to-implement-equals-and-hashcode-using-the-jpa-entity-identifier/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, about the null subject, we have the Column annotation with the nullable = false. Furthermore, I think we could ensure more the type-safetiness using the @NotNull annotation.

Good point. I was thinking that it is better to make an issue to make all validation regards the entities level

private List<Team> teams = new ArrayList<>();

public void addTeam(Team team) {
if (teams == null) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this isn't needed because the teams list is initialized always before the constructor call:
private List teams = new ArrayList<>();

We don't have any teams setter to be afraid (and we shouldn't) to avoid setTeams(null). In the future, if needed, a removeTeam method should be added.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is true if we are not using the builder function. Otherwise, the teams would be null.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not understand this part:

We don't have any teams setter to be afraid (and we shouldn't) to avoid setTeams(null). In the future, if needed, a removeTeam method should be added.

Wasn't this what was previously implemented? I thought it is what you meant with the comment about bidirectional relations. What am I missing?

teams = new ArrayList<>();
}

if (!teams.contains(team)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To avoid duplicates, and this check, we could just select a collection that doesn't allow duplicates:
private Set teams = new LinkedHashSet<>();

@mikededo mikededo Dec 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, that's what I thought of using. However, is there any configuration with JPA to change? Or it will automatically manage it?

@mikededo
mikededo requested review from a team and plozanol December 12, 2022 14:27
@plozanol
plozanol merged commit 54b897e into main Dec 13, 2022
@plozanol
plozanol deleted the fix/change-entity-association branch December 13, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: code review PR ready to be reviewed type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: change entity association in UserControllerOption

2 participants