Trunk 5866 : OrderSet Domain - Switching from Hibernate Mappings to Annotations #5472
Conversation
sudhanshu-raj
changed the title
sudhanshu-raj
force-pushed
the
TRUNK-5866
branch
from
0a37ed4 to
250d90e
Compare
|
Hey , I did the change , it should build success now . |
sudhanshu-raj
force-pushed
the
TRUNK-5866
branch
from
250d90e to
87cbb74
Compare
|
Hey @dkayiwa , can you please review this ? |
|
Can you include a link to the ticket id as advised at https://openmrs.atlassian.net/wiki/display/docs/Pull+Request+Tips? |
|
Done ! |
| private Operator operator; | ||
|
|
||
|
|
||
| @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true) |
There was a problem hiding this comment.
How did you deal with the lazy attribute?
There was a problem hiding this comment.
I think OneToMany has default fetch type lazy , ref : https://stackoverflow.com/questions/26601032/default-fetch-type-for-one-to-one-many-to-one-and-one-to-many-in-hibernate
There was a problem hiding this comment.
Do you think you can come up with a test in https://github.com/openmrs/openmrs-core/blob/master/api/src/test/java/org/openmrs/api/OrderSetServiceTest.java to catch this problem?
As in a test which would fail when using AttributeOverride but passes with AssociationOverride
| } | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "order_set_id_seq") |
There was a problem hiding this comment.
What do you think of using GenerationType.IDENTITY?
There was a problem hiding this comment.
In orderset.hbm.xml sequence has been used , so that's why not using identity here .
There was a problem hiding this comment.
@wikumChamith what is our recommended pattern for this?
There was a problem hiding this comment.
We just use IDENTITY as the strategy
There was a problem hiding this comment.
Thanks , this is helpful
sudhanshu-raj
force-pushed
the
TRUNK-5866
branch
2 times, most recently
from
1a62eeb to
c022deb
Compare
| @Table(name = "order_set") | ||
| @Audited | ||
| @AssociationOverride( | ||
| name = "attributes", |
There was a problem hiding this comment.
This overrides the join column name for attributes field ,actually in the OrderSet's parent class which is BaseCustomizableMetadata it has join column name location_id but in orderset.hbm.xml it has order_set_id join column name so it's overriding that join column name from it's parent.
There was a problem hiding this comment.
Did you evaluate using AttributeOverride?
There was a problem hiding this comment.
AttributeOverride works well for normal fields not for relationship attributes so here AssociationOverride will work for relationships columns like OneToMany
There was a problem hiding this comment.
Even when you are not modifying a relationship but just renaming a filed like here?
There was a problem hiding this comment.
yes , this just renaming the column name , it's just can change how it should be mapped to databased like joincolumn name or jointable name , but can't actually modify the relationship like onetomany to manytoone , fetch type etc
There was a problem hiding this comment.
Doesn't that then mean AttributeOverride is enough for this use case?
There was a problem hiding this comment.
I don’t think @AttributeOverride works here. It’s used for simple attributes for example, renaming a column on a basic field like @Column(name = "idt") private String identifier; or for fields inside an @Embeddable. It only changes how a single column is named or mapped.
But for relationships like @OneToMany or @ManyToOne, we use @AssociationOverride. That’s what lets us change the name of the join column or join table for an existing relation.
This is what I understand, please let me know if I’m missing something.
There was a problem hiding this comment.
What is the difference between this and https://github.com/openmrs/openmrs-core/blob/2.8.1/api/src/main/java/org/openmrs/Location.java#L52 ?
There was a problem hiding this comment.
The @AttributeOverride(name = "attributes", column = @column(name = "location_id")) annotation on the Location entity is not doing anything meaningful and can be safely removed. Why ? because :@AttributeOverride only works for simple fields (String, Integer, Date, etc.), not relationships like @onetomany
In BaseCustomizableMetadata, the attributes field is a relationship (@onetomany), not a basic column:
@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true, fetch = FetchType.LAZY)
@JoinColumn(name = "location_id")
private Set<A> attributes = new LinkedHashSet<>();Since @AttributeOverride cannot override relationships, it gets silently ignored during entity mapping. The application runs fine without it which i also verified .
Ref : https://www.baeldung.com/jpa-attributeoverride
Recommendation: Remove the @AttributeOverride annotation from Location as it serves no purpose.
sudhanshu-raj
force-pushed
the
TRUNK-5866
branch
from
c022deb to
206cad7
Compare
3 participants
…notations
Description of what I changed
Issue I worked on
see https://openmrs.atlassian.net/browse/TRUNK-5866
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master