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.

Sign up for GitHub

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

Jump to bottom

MIGENG-417 replacing One-To-One with a Many-to-One #75

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

MIGENG-417 replacing One-To-One with a Many-to-One #75

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cdf6246
MIGENG-365 - End2end integration test (#20)
Jan 9, 2020
953a97c
Migeng 327 avoid crashes based on issues use cases (#68)
Jan 22, 2020
9677740
hotfix-kafka-autooffset set autoOffsetReset to latest (#71)
Jan 22, 2020
4efb927
MIGENG-114 Remove useless routes (#64)
mrizzi Jan 31, 2020
3aecd4e
hotfix S3 bucket was hardcoded on the payload endpoint (#72)
Jan 31, 2020
186a847
MIGENG-417 replacing One-To-One with a Many-to-One
Feb 4, 2020
1d29aa8
MIGENG-417 replacing One-To-One with a Many-to-One
Feb 4, 2020
11ecbfe
Merge branch 'master' into MIGENG-417-fix-cardinality
Feb 25, 2020
f14ff2d
Fixed conflicts
Feb 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...g/jboss/xavier/analytics/pojo/output/workload/inventory/WorkloadInventoryReportModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import javax.persistence.Id;
import javax.persistence.Index;
import javax.persistence.JoinColumn;
import javax.persistence.OneToOne;
import javax.persistence.ManyToOne;
import javax.persistence.Table;
import javax.transaction.Transactional;
import java.util.Date;
Expand Down Expand Up @@ -65,7 +65,7 @@ public class WorkloadInventoryReportModel
)
private Long id;

@OneToOne(fetch = FetchType.LAZY)
@ManyToOne(fetch = FetchType.LAZY)
Copy link
Member

Choose a reason for hiding this comment

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

@jonathanvila Since you are enhancing this relation I suggest we can add optional = false so we are 100% sure that there won't exist any WorkloadInventoryReportModel without an AnalysisModel. this change should add not null constraint to the column analysis_id into the workload_inventory_report_model table.

Suggested change
@ManyToOne(fetch = FetchType.LAZY)
@ManyToOne(fetch = FetchType.LAZY, optional = false)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it won't make any harm, but it's totally impossible to have a null there as it has a foreign key against analysis_id where id is the primary therefore not null
"fkt966e5qfj0hwmb0oecel9qbyw" FOREIGN KEY (analysis_id) REFERENCES analysis_model(id)

Copy link
Member

@carlosthe19916 carlosthe19916 Feb 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@jonathanvila Let me clarify my previous comment. Right now, we have a foreign key in workload_inventory_report_model which points to analysis_model; however, having a foreign key in workload_inventory_report_model does not force the table to contain not null values into the column analysis_id. For example you can execute these commands:

insert into workload_inventory_report_model(id) values (-1);
select * from workload_inventory_report_model where id=-1;

Screenshot from 2020-02-06 17-12-50

You will see that the previous command can be executed; it means: "I've created a workload_inventory_report_model without an analysis_model"

This is the current definition of the table, and as you can see the column analysis_id accept null values.

Screenshot from 2020-02-06 17-13-45

I think we missed this kind of small detail in many of our current Models; however, we can improve it by the time so we have concise model definitions.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right.
We should revisit our whole model design .... obviously that foreign key should not be nullable
This concerns me more then, that we can find other issues in the model :(

So I would suggest not doing this small change you suggests in this PR, but open another issue of revisiting the whole model adding this and all other changes needed

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's create a ticket for analyzing all our models.

@JoinColumn(name = ANALYSIS_ID)
@JsonBackReference
private AnalysisModel analysis;
Expand Down