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

Add support for forein keys in schema generation within aggregates #1629

Closed
wants to merge 4 commits into from

Conversation

kobaeugenea
Copy link
Contributor

@kobaeugenea kobaeugenea commented Sep 30, 2023

Done:

  • Generate column and foreign key for @MapedCollection annotated fields
  • Generate only foreign key for @MapedCollection annotated fields if column exists in child entry
  • Drop foreign keys
  • Foreign key name generator (hardcoded now)

Always drop foreign keys before table drop. Always add foreign keys after table creation. The same for fields. (to be sore that referenced column always exists).

Closes #1599
Related tickets #756, #1600

Copy link
Contributor

@schauder schauder left a comment

Choose a reason for hiding this comment

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

This looks pretty good already.

I see just one major problem: It won't work for nested lists or maps.
If you could add a test for those and fix the resulting issues it would be ready to go.

@kobaeugenea
Copy link
Contributor Author

kobaeugenea commented Oct 14, 2023

Hey! I did some polishing and added the test for nested collections. Please give me more information if I got your comment wrong.

@kobaeugenea kobaeugenea requested a review from schauder October 14, 2023 20:02
schauder pushed a commit that referenced this pull request Oct 19, 2023
Original pull request #1629
@schauder
Copy link
Contributor

I'm sorry, I wasn't very precise when I said nested collections.

What I really should have said is nested lists or maps where the entities don't have ids.

I modified the domain model of your test to clarify what I mean: https://github.com/spring-projects/spring-data-relational/tree/pr/schema-generation-fks

@kobaeugenea
Copy link
Contributor Author

I'm sorry, I wasn't very precise when I said nested collections.

What I really should have said is nested lists or maps where the entities don't have ids.

I modified the domain model of your test to clarify what I mean: https://github.com/spring-projects/spring-data-relational/tree/pr/schema-generation-fks

@schauder Oh, sorry don't quite understnad that case. I thought that every entity (class marked by Table annotation) must have field with Id annotation. If not I can't see where foreign key of OtherTable should reference in Tables. I can suggest that it can reference to composite key using all fields of Tables (then it makes sense to use collection of column names). But in that case why do you talk only about Map and List but not about Set?
Diffrence I know about Map/List and Set is MappedCollection for Map/List must have 'keyColumn' but it is for ordering as I understand and don't do anything with primary keys/foreign keys.

Sorry if my questions are stupid, just something doesn't match for me =)

@schauder
Copy link
Contributor

Not at all stupid questions. Inner entities of 1:1 relations or in lists or maps don't need ids. I actually recommend against ids.
The id of the aggregate root + the keycolumns for the lists/maps form a primary key.

You can see examples of this in


And the matching schema in

@kobaeugenea
Copy link
Contributor Author

@schauder Oh, okay, now I see. Please check my last commit, I fixed it.

Just one questions to ensure. Is my guess right, that if parent entity already have composite primary key than "idColumn" of MappedCollection doesn't work and child entity reuses names from parent without changing (only adding keyColumn to primary key)?

Please look at top level entity (ListOfMapOfNoIdTables) in tests. I used "idColumn" there. Can you please check that primary key column names are correct in MapOfNoIdTables and NoIdTable based on that annotation.

@kobaeugenea
Copy link
Contributor Author

kobaeugenea commented Oct 21, 2023

@schauder Hey! In last commit I added processing one-to-one relations, also simplified code a little, fixed bugs and added few tricky test cases.
Please take a look on
createForeignKeyForOneToOneWithMultipleChildren
createForeignKeyForCircularWithId
createForeignKeyForCircularNoId
test cases. Escpecially I'm confused about this kind of entities

       @org.springframework.data.relational.core.mapping.Table
	static class OneToOneLevel2 {
		NoIdTable table1;
		@Column("additional_one_to_one_level2")
		NoIdTable table2;
	}

What is primary key in NoIdTable? one_to_one_level1 or additional_one_to_one_level2 or both or none? Because primary key is not nullable but what if one of the referencing fields will be null? Now I just pick first one available and make it to be primary key. Maybe we don't need to create primary key at all in such cases?

Here is even more interesting example:

       @org.springframework.data.relational.core.mapping.Table
	static class ParentOfCircularNoId {
		@Id int id;
		CircularNoId CircularNoId;

	}

	@org.springframework.data.relational.core.mapping.Table
	static class CircularNoId {
		CircularNoId CircularNoId;
	}

What schema should be generated here?

@schauder
Copy link
Contributor

If there are multiple child references of the same type and that type doesn't have an id, we strictly speaking shouldn't create PK. But I would be willing to fix this with a later change.

@schauder
Copy link
Contributor

Circular references aren't allowed and should fail, preferable with an expressive exception.

@schauder schauder added the has: design-decision An issue that contains a design decision about its topic label Nov 23, 2023
@schauder
Copy link
Contributor

Just one questions to ensure. Is my guess right, that if parent entity already have composite primary key than "idColumn" of MappedCollection doesn't work and child entity reuses names from parent without changing (only adding keyColumn to primary key)?

Please look at top level entity (ListOfMapOfNoIdTables) in tests. I used "idColumn" there. Can you please check that primary key column names are correct in MapOfNoIdTables and NoIdTable based on that annotation.

Yeah, that looks correct.

schauder pushed a commit that referenced this pull request Nov 24, 2023
schauder added a commit that referenced this pull request Nov 24, 2023
Removed tests for circular references since they are not supported by Spring Data Relational.

Closes #1599
See #756, #1600
Original pull request #1629
@schauder
Copy link
Contributor

I merged this after removing the tests for circular references, since these are not supported anyway.

Thanks.

@schauder schauder closed this Nov 24, 2023
@schauder schauder added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has: design-decision An issue that contains a design decision about its topic type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for forein keys in schema generation within aggregates
3 participants