When an aggregate root has no id we should throw an appropriate exception

[schauder]

Currently Spring Data JDBC complains that the id is still null after save.

[AndreynRosa]

May I take this to try to contribute?

[schauder]

Sure, go ahead.

[AndreynRosa]

@schauder I don't know if this is the right place to ask these questions... But, i have a question. When you say "save", do you mean an insert action? Can i have more details?

[schauder]

Yes. There is a check in place that after saving a new aggregate, i.e. inserting its aggregate root, the id is not null. This will obviously fail when there is no id property in the first place.

See https://github.com/spring-projects/spring-data-relational/blob/fbc3bf09e1b93ce887b230354dc54376e002007e/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateTemplate.java#L453

But this is not the place where we should check for the existence of an id. I think we should do that when constructing a repository, because then we know a entity is intended to be used as an aggregate root and therefore should have an id property.

[AndreynRosa]

Hi, @schauder. I'm still here trying to fix this issue. Haha! I think the correct place to verify if an entity has an identifier is in the JdbcRepositoryFactory, which is responsible for creating a new repository. Is that correct? Should I perform this verification in the constructor?

in this method:

       public JdbcRepositoryFactory(DataAccessStrategy dataAccessStrategy, RelationalMappingContext context,
        JdbcConverter converter, Dialect dialect, ApplicationEventPublisher publisher,
        NamedParameterJdbcOperations operations) {

    Assert.notNull(dataAccessStrategy, "DataAccessStrategy must not be null");
    Assert.notNull(context, "RelationalMappingContext must not be null");
    Assert.notNull(converter, "RelationalConverter must not be null");
    Assert.notNull(dialect, "Dialect must not be null");
    Assert.notNull(publisher, "ApplicationEventPublisher must not be null");

    this.publisher = publisher;
    this.context = context;
    this.converter = converter;
    this.dialect = dialect;
    this.accessStrategy = dataAccessStrategy;
    this.operations = operations;
}

[schauder]

I would put this check in the JdbcAggregateTemplate or in the classes called by it.

Putting it in the JdbcRepositoryFactory hat two problems:

1. It would prevent people from (ab)using repositories for stuff that does not require the id. While I'm not completely against preventing this, now is to late, since a lot of people are probably already doing it.
2. You don't have to use a repository. You may use a JdbcAggregateTemplate directly.

Here is how I would approach this:

1. Find an integration test where you can add a test, based on the JdbcAggregateTemplate.
2. Create a testcase, that currently should fail with a complaint as described in the original description of the issue.
3. Check where that exception is coming from. Replace it with a proper exception, describing the lack of an id.

[AndreynRosa]

Your explanations helped me a lot. I think I am done, but I don't know if it's right. Should I open a Merge Request for you to see if it's ok? Must I follow the steps in this document, https://github.com/spring-projects/spring-data-build/blob/main/CONTRIBUTING.adoc ?

[schauder]

Yes, a PR would be the next step. And yes that is the correct document for guidance. But we are already did a lot of the stuff (like discussing changes in an issue) and other stuff does not apply (the stuff about security issues).

So the stuff to look out for:

• Don't mess up the existing formatting.
• For new stuff apply our style of formatting
• Make sure there are tests.

See also this section: https://github.com/spring-projects/spring-data-build/blob/main/CONTRIBUTING.adoc#quickstart

Everything als I can and will fix in post.