Initial Data Model for Schema SQL Generation #1481

kurtn718 · 2023-04-05T13:16:28Z

Initial data model for Schema SQL Generation.

If we want to have a feature branch (instead of main) that these changes get merged into, I am definitely good with that approach. Please let me know.

You have read the Spring Data contribution guidelines.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Initial model for generating schema SQL. Closes #1478

schauder

A change like this is hard to review since it doesn't really create a usable feature.
For a better change "usable" can be considered rather broadly. For example I'd consider a schema generator that creates a schema consisting of a single table with correct name for each aggregate and with a single dummy column usable. It can be invoked by a user and produces something at least somehow useful.

In such a change one can take a look at the underlying model and form an opinion if the underlying model is fit for purpose.
With the current model this is mostly guess work.

As for the merge target: We should not merge into main until we have something that is really usable for the enduser. Basically something we/you would be willing to present as a new feature of Spring Data JDBC.

Until we have that everything should happen on the feature branch, and we don't need formal PRs.

Let's discuss further details about how to proceed in person.

schauder · 2023-04-10T15:47:21Z

...src/main/java/org/springframework/data/relational/core/mapping/RelationalMappingContext.java

+	 * Method to build a data model of Tables/Columns and their relationships
+	 * that will be used in SQL Generation.
+	 */
+	public SchemaSQLGenerationDataModel generateSchemaSQL() {


I don't think the mapping context should create a SchemaSqlGenerationDataModel instead a mapping context should be used to create such a model. Either in the constructor of the class or in a factory.

About to push up a change for this. Code definitely looks cleaner with this!

schauder · 2023-04-10T15:49:02Z

...n/java/org/springframework/data/relational/core/mapping/schemasqlgeneration/ColumnModel.java

+    private Class<?> type;
+    private boolean nullable;
+
+    public String getName() {


Things like column and table names should be SqlIdentifier since we need to consider quoting and identifier processing.

schauder · 2023-04-10T15:51:40Z

...n/java/org/springframework/data/relational/core/mapping/schemasqlgeneration/ColumnModel.java

+        this.name = name;
+    }
+
+    public Class<?> getType() {


I don't see why we would need a type in the sense of a Class. This information is provided by PersistentProperty. Instead we might need a type like Text or Number which represents database types, while abstracting over the database specific representations of those. Possibly by using Liquibase abstractions, if those exist.

...n/java/org/springframework/data/relational/core/mapping/schemasqlgeneration/ColumnModel.java

schauder · 2023-04-10T15:55:54Z

...g/springframework/data/relational/schemasqlgeneration/SchemaSQLGenerationDataModelTests.java

+    }
+
+    @Table
+    static class Luke {


The choice of testdata names doesn't help to understand why these look the way they do look.

I consider names like EntityWithMultipleColumns or stringProperty and so one more expressive.

schauder · 2023-04-10T15:56:30Z

...g/springframework/data/relational/schemasqlgeneration/SchemaSQLGenerationDataModelTests.java

+
+        TableModel t1 = model.getTableData().get(0);
+        assertThat(t1.getName()).isEqualTo("luke");
+        assertThat(t1.getColumns().get(0).getName()).isEqualTo("force");


This and the following three assertions basically test the same thing.

…erializable

kurtn718 · 2023-04-11T17:55:08Z

Going to convert the Pull Request to a Draft

…alMappingContext

kurtn718 · 2023-04-11T23:35:28Z

...org/springframework/data/relational/core/mapping/schemasqlgeneration/SchemaSQLGenerator.java

+public class SchemaSQLGenerator {
+
+    private final IdentifierProcessing identifierProcssing;
+    public SchemaSQLGenerator(IdentifierProcessing identifierProcessing) {


@schauder -

Is there a way to get the IdentifierProcessing from the RelationalMappingContext? (or when user's use this class to generate SQL - that they can easily get an IdentifierProcessing object - that matches the one they are using for executing SQL)?

For the Unit Test(s) I think passing it in gives flexibility.

kurtn718 · 2023-04-11T23:40:14Z

...n/java/org/springframework/data/relational/core/mapping/schemasqlgeneration/ColumnModel.java

@@ -29,16 +29,16 @@ public class ColumnModel implements Serializable {
    @Serial
    private static final long serialVersionUID = 1L;
    private final SqlIdentifier name;
-    private final Class<?> type;
+    private final String type;


Decided to simplify the model to be a String value. Whatever is in here is what gets in the generated SQL.

kurtn718 · 2023-04-11T23:49:59Z

...framework/data/relational/core/mapping/schemasqlgeneration/SchemaSQLGenerationDataModel.java

@@ -56,7 +62,9 @@ public SchemaSQLGenerationDataModel(RelationalMappingContext context) {

            while (iter.hasNext()) {
                BasicRelationalPersistentProperty p = iter.next();
-                ColumnModel columnModel = new ColumnModel(p.getColumnName(), p.getActualType());
+                ColumnModel columnModel = new ColumnModel(p.getColumnName(),
+                        typeMapper.databaseTypeFromClass(p.getActualType()),


@schauder - I know initially I had wanted to modify the @column annotation to have a gazillion modifiers :-) to match what was in JPA.

After implementing the default behavior (and the change to the Column model) - I think adding just one annotation (databaseType) would be sufficient. We would let developers specify whatever they want there, and that is exactly what would be in the generated SQL.

If the user wishes to change our defaults, they can optionally subclass BaseTypeMapper. I stayed with what I think are datatypes that are supported by all Databases.

Will add more comments to the code - to document/describe to users how to extend/change behavior.

Initial Data Model for Schema SQL Generation that can generate SQL for a simple entity. Closes #1478

kurtn718 added 2 commits April 5, 2023 09:02

1478 - Prepare branch

a30d01a

Model fot Generating Schema SQL

9a7bb2b

Initial model for generating schema SQL. Closes #1478

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 5, 2023

kurtn718 requested a review from schauder April 5, 2023 13:20

schauder requested changes Apr 10, 2023

View reviewed changes

kurtn718 added 2 commits April 10, 2023 19:29

Changes to make model attributes final and declare model classes as S…

38aeaaf

…erializable

Use SQLIdentifiers for Table and Column names

c59f8a7

kurtn718 marked this pull request as draft April 11, 2023 17:55

kurtn718 added 3 commits April 11, 2023 14:14

Move logic for generating Schema Generation model outside of Relation…

e183e9e

…alMappingContext

Changes to generate/populate type on Column model

5762a06

Initial SQL generation (no aggregates / default types)

985b703

kurtn718 commented Apr 11, 2023

View reviewed changes

kurtn718 changed the base branch from main to feature/schema-generation April 13, 2023 11:53

Merge branch 'feature/schema-generation' into issue/1478

aed2adf

kurtn718 marked this pull request as ready for review April 13, 2023 11:56

kurtn718 merged commit ad7b087 into feature/schema-generation Apr 13, 2023

kurtn718 deleted the issue/1478 branch April 13, 2023 11:59

kurtn718 added a commit that referenced this pull request May 20, 2023

Initial Data Model for Schema SQL Generation (#1481)

f5fa502

Initial Data Model for Schema SQL Generation that can generate SQL for a simple entity. Closes #1478

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Data Model for Schema SQL Generation #1481

Initial Data Model for Schema SQL Generation #1481

kurtn718 commented Apr 5, 2023 •

edited

Loading

schauder left a comment

schauder Apr 10, 2023

kurtn718 Apr 11, 2023

schauder Apr 10, 2023

schauder Apr 10, 2023

schauder Apr 10, 2023

schauder Apr 10, 2023

kurtn718 commented Apr 11, 2023

kurtn718 Apr 11, 2023

kurtn718 Apr 11, 2023

kurtn718 Apr 11, 2023

Initial Data Model for Schema SQL Generation #1481

Initial Data Model for Schema SQL Generation #1481

Conversation

kurtn718 commented Apr 5, 2023 • edited Loading

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtn718 commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtn718 commented Apr 5, 2023 •

edited

Loading