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

Entities without @ID fail to insert and count #672

Closed
marioburatto opened this issue Oct 26, 2021 · 4 comments
Closed

Entities without @ID fail to insert and count #672

marioburatto opened this issue Oct 26, 2021 · 4 comments
Assignees
Labels
status: superseded An issue that has been superseded by another

Comments

@marioburatto
Copy link

marioburatto commented Oct 26, 2021

I have a Many-To-Many relation entity that fails to insert and count because R2dbcEntityTemplate requires a mandatory idProperty.

I came to this issue while trying to workaround the lack of a Multi Id / Composite Id feature. The solution I found was have no @id annotation and rely on the high flexibility provided by R2dbcEntityTemplate to create my repository (which was very easy by the way, as you can see here).

The limitation I found was that part of the R2dbcEntityTemplate implementation forces a required idProperty with no reason for that during the insert and count operations. Please see bellow the details:

Insert Failure
As I'm using Postgres, all the insert statements have a "RETURNING" clause appended to the operation. When there is no ID, it appends "RETURNING *". With these data being returned back during the insert operation the entity template tries to populateIdIfNecessary, but instead of just ignoring it because it's not necessary, it fails complaining the required idProperty. In the way I see, MappingR2dbcConverter could simply skip populateIdIfNecessary when the idProperty is null:

RelationalPersistentProperty idProperty = entity.getIdProperty();
if(idProperty == null){
    return object;
}

Count Failure
During the doCount operation R2dbcEntityTemplate submits a Function.count() to the projection and it fails because it requires the idProperty to be counted. The same issue supposed to happen with doExists operation, but the idProperty was already made optional there using a "*" field instead. I guess the same could be applied here by projecting a count(*) when there is no idProperty, like this:

SqlIdentifier columnName = entity.hasIdProperty() ? entity.getRequiredIdProperty().getColumnName()
                : SqlIdentifier.unquoted("*");

StatementMapper.SelectSpec selectSpec = statementMapper //
        .createSelect(tableName) //
        .doWithTable((table, spec) -> {
            return spec.withProjection(Functions.count(table.column(columnName)));
        });

If you want to, I can submit a PR with the fixes and a couple test cases.

Please let me know your thoughts.

Thanks, Mario.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 26, 2021
@mp911de
Copy link
Member

mp911de commented Oct 26, 2021

Paging @schauder. Generally, I think that would work. Your case seems to describe an entity with a composite primary key that isn't supported properly yet.

@schauder
Copy link
Contributor

If one looks at it from a single entity point of view, I agree, it should be considered a single entity with a multi column id.
This currently isn't supported but frequently requested.
This specific case will still have the problem that Spring Data can't easily if the entity is new or existing when the full id is not null, but that can be solved without changing the entity.

If one takes the Spring Data JDBC perspective the entity should be part of an aggregate, with a unique key consisting of the back reference to the root and all values, which in this case is just a single value pointing to the other aggregate.
But full aggregates aren't supported yet for R2DBC.

If someone would find the time to implement multipart ids, that would be highly appreciated.

See: spring-projects/spring-data-relational#574

@marioburatto
Copy link
Author

Thanks @mp911de and @schauder for the feedback. I tried to add a little more details of my issue to the description.

With regards to the insert issue it generally works, but fails when the database returns the inserted rows.

If you add this test case to the R2dbcEntityTemplateUnitTests you will be able to reproduce it.


	@Test // gh-672
	void shouldInsertAnEntityWithNoIdAnnotation() {

		MentoringRelation mentoring = new MentoringRelation(new Person(), new Person(), LocalDateTime.now());
		mentoring.mentor.id = "1";
		mentoring.mentee.id = "2";

		RowMetadata rowMetadata = MockRowMetadata.builder()
				.columnMetadata(MockColumnMetadata.builder().name("start_date").build())
				.columnMetadata(MockColumnMetadata.builder().name("mentor_id").build())
				.columnMetadata(MockColumnMetadata.builder().name("mentee_id").build())
				.build();
		Row row = MockRow.builder()
				.identified("start_date", LocalDateTime.class, LocalDateTime.now())
				.identified("mentor_id", Object.class, 1)
				.identified("mentee_id", Object.class, 2)
				.build();

		//Add customConverter
		entityTemplate = new R2dbcEntityTemplate(
				entityTemplate.getDatabaseClient(),
				new DefaultReactiveDataAccessStrategy(
						PostgresDialect.INSTANCE,
						Collections.singleton(new MentoringRelationWriteConverter()))
		);

		MockResult result = MockResult.builder()
				.rowMetadata(rowMetadata)
				.row(row)
				.build();

		recorder.addStubbing(s -> s.startsWith("INSERT"), result);

		entityTemplate
				.insert(mentoring)
				.as(StepVerifier::create) //
				.assertNext(actual -> {
					assertThat(actual.mentor.id).isEqualTo("1");
					assertThat(actual.mentee.id).isEqualTo("2");
				}) //
				.verifyComplete();

		StatementRecorder.RecordedStatement statement = recorder.getCreatedStatement(s -> s.startsWith("INSERT"));

		assertThat(statement.getSql()).isEqualTo("INSERT INTO mentoring (start_date, mentor_id, mentee_id) VALUES ($1, $2, $3)");
	}

	@Value
	@With
	@Table("mentoring")
	static class MentoringRelation {
		Person mentor;
		Person mentee;
		LocalDateTime startDate;
	}

	@WritingConverter
	static class MentoringRelationWriteConverter implements Converter<MentoringRelation, OutboundRow> {

		public OutboundRow convert(MentoringRelation source) {
			OutboundRow row = new OutboundRow();
			row.put("start_date", Parameter.from(source.getStartDate()));
			row.put("mentor_id", Parameter.from(source.getMentor().id));
			row.put("mentee_id", Parameter.from(source.getMentee().id));
			return row;
		}
	}

Please let me know your thoughts.

Thanks, Mario.

@mp911de mp911de self-assigned this Feb 14, 2022
@mp911de mp911de added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 2, 2022
@mp911de
Copy link
Member

mp911de commented Mar 2, 2022

That issue was fixed with #711 since version 1.3.9.

@mp911de mp911de closed this as completed Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

4 participants