Skip to content

Criteria "in" with UUID throws exception / findAllById #283

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

Closed
Dav1dde opened this issue Jan 22, 2020 · 14 comments
Closed

Criteria "in" with UUID throws exception / findAllById #283

Dav1dde opened this issue Jan 22, 2020 · 14 comments
Labels
status: waiting-for-feedback We need additional information before we can continue type: bug A general bug

Comments

@Dav1dde
Copy link

Dav1dde commented Jan 22, 2020

A simple setup with a UUID as id:

class Entity {
    @Id
    @Column("ID")
    private UUID id;
}
@Repository
class Repo extends ReactiveCrudRepository<Entity, UUID> {
}

The following fails repo.findAllById(List.of(existingUuid))
The same happens using the database client: client.select().from(Entity.class).matching(Criteria.where("id").in(List.of(existingUuid))).fetch().all()

Error: java.lang.IllegalArgumentException: Cannot encode null parameter of type java.lang.Object

java.lang.IllegalArgumentException: Cannot encode null parameter of type java.lang.Object
	at io.r2dbc.postgresql.codec.DefaultCodecs.encodeNull(DefaultCodecs.java:155) ~[r2dbc-postgresql-0.8.0.RELEASE.jar:0.8.0.RELEASE]
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Stack trace:
		at io.r2dbc.postgresql.codec.DefaultCodecs.encodeNull(DefaultCodecs.java:155) ~[r2dbc-postgresql-0.8.0.RELEASE.jar:0.8.0.RELEASE]
		at io.r2dbc.postgresql.ExtendedQueryPostgresqlStatement.bindNull(ExtendedQueryPostgresqlStatement.java:113) ~[r2dbc-postgresql-0.8.0.RELEASE.jar:0.8.0.RELEASE]
		at io.r2dbc.postgresql.ExtendedQueryPostgresqlStatement.bindNull(ExtendedQueryPostgresqlStatement.java:45) ~[r2dbc-postgresql-0.8.0.RELEASE.jar:0.8.0.RELEASE]
		at org.springframework.data.r2dbc.core.DefaultDatabaseClient$StatementWrapper.bindNull(DefaultDatabaseClient.java:1579) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.dialect.IndexedBindMarker.bindNull(IndexedBindMarker.java:56) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.dialect.Bindings$NullBinding.apply(Bindings.java:281) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.dialect.Bindings.lambda$apply$1(Bindings.java:122) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at java.base/java.util.LinkedHashMap.forEach(LinkedHashMap.java:684) ~[na:na]
		at org.springframework.data.r2dbc.dialect.Bindings.apply(Bindings.java:122) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.core.DefaultStatementMapper$DefaultPreparedOperation.bindTo(DefaultStatementMapper.java:314) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.core.DefaultDatabaseClient$ExecuteSpecSupport.lambda$exchange$1(DefaultDatabaseClient.java:341) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at org.springframework.data.r2dbc.core.DefaultDatabaseClient.lambda$null$17(DefaultDatabaseClient.java:1429) ~[spring-data-r2dbc-1.0.0.RELEASE.jar:1.0.0.RELEASE]
		at reactor.core.publisher.FluxDefer.subscribe(FluxDefer.java:46) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]
		at reactor.core.publisher.Flux.subscribe(Flux.java:8134) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]
		at reactor.core.publisher.FluxUsingWhen$ResourceSubscriber.onNext(FluxUsingWhen.java:201) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]
		at org.springframework.security.test.context.support.ReactorContextTestExecutionListener$DelegateTestExecutionListener$SecuritySubContext.onNext(ReactorContextTestExecutionListener.java:105) ~[spring-security-test-5.2.1.RELEASE.jar:5.2.1.RELEASE]
		at reactor.core.publisher.FluxMap$MapSubscriber.onNext(FluxMap.java:114) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]
		at reactor.core.publisher.FluxOnErrorResume$ResumeSubscriber.onNext(FluxOnErrorResume.java:73) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]
		at reactor.core.publisher.FluxOnErrorResume$ResumeSubscriber.onNext(FluxOnErrorResume.java:73) ~[reactor-core-3.3.0.RELEASE.jar:3.3.0.RELEASE]

One of the problems here is (from SimpleR2dbcRepository):

        StatementMapper.SelectSpec selectSpec = mapper.createSelect(this.entity.getTableName()) //
            .withProjection(columns) //
            .withCriteria(Criteria.where(idColumnName).in(ids));

idColumnName contains ID instead of id (the value from the @Column annotation), if I change the value to "id", I do not get an exception but this warning:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.springframework.util.ReflectionUtils (file:/home/dav1d/.m2/repository/org/springframework/spring-core/5.2.1.RELEASE/spring-core-5.2.1.RELEASE.jar) to constructor java.util.UUID(byte[])
WARNING: Please consider reporting this to the maintainers of org.springframework.util.ReflectionUtils
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

And no results. But if I change the criteria from .in to .is the entity is found.

@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

Thanks for report. These are two issues. One is that we use mapped and unmapped operations in the Repository but always use the column name instead of using the property name where we employ mapping.

The other issue is that UUID gets inspected as entity type and that is why you see the illegal access warning.

@IvanKonevJr
Copy link

Affirmative, in QueryMapper used this.converter.writeValue(o, typeInformation.getActualType() -- it is clear source object type

@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

I wasn't able to reproduce the exception above. From the stack trace it looks as if a null value is being bound. Calling findAllById(Collections.singletonList(null)) fails with a Reactor exception first as null values are prohibited in Reactive Streams and we're deconstructing the incoming ID collection via Flux.fromIterable(…).

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Jan 23, 2020
@Dav1dde
Copy link
Author

Dav1dde commented Jan 23, 2020

@mp911de the UUID was not null that's why I was so confused at first, I passed in a valid uuid that does exist in the database. But in org.springframework.data.r2dbc.query.QueryMapper#getCondition the propertyType.getTypeHint() call into org.springframework.data.r2dbc.query.QueryMapper.MetadataBackedField#getTypeHint returns super.getTypeHint() which returns Object, now UUID and Object don't match and null will be bound because the conversion returns null

@Dav1dde
Copy link
Author

Dav1dde commented Jan 23, 2020

The reason why it returns super.getTypeHint() is because property is null, this is because in org.springframework.data.r2dbc.query.QueryMapper.MetadataBackedField ctor, getPath(name) returns null because name is ID not id (latter being the field name of the Entity)

@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

There's a bit more to reproduce it. Since UUID was considered an entity type, BasicRelationalConverter tried to represent it as an entity and so the null value was caused.

@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

I filed DATAJDBC-476 to change the preference for conversion so that unknown properties are attempted for simple type conversion first and the number of cases where a valid value can be converted to null gets reduced.

mp911de added a commit that referenced this issue Jan 23, 2020
We now use the Id property name when using the converter to map property names to column names. In other places, where we don't use the converter, we stick with the Id column name.
mp911de added a commit that referenced this issue Jan 23, 2020
We now use the Id property name when using the converter to map property names to column names. In other places, where we don't use the converter, we stick with the Id column name.
@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

That's fixed and backported now.

@mp911de mp911de closed this as completed Jan 23, 2020
@IvanKonevJr
Copy link

IvanKonevJr commented Jan 23, 2020

i found isAssignableFrom in codec detect logic - it is a 2 times slower that instanceOf or isInstance, primitive types as int not used in models and isAssignableFrom case is a support primitive types

@Dav1dde
Copy link
Author

Dav1dde commented Jan 23, 2020

@mp911de this still suffers from the reflection warning and the query won't find any rows (even though it should). Or is this caused by DATAJDBC-476?

@mp911de
Copy link
Member

mp911de commented Jan 23, 2020

Not sure I follow @Dav1dde. UUID is no longer considered an entity type with #284. We also cleaned up property/column references in the scope of this ticket. The change is available from the most recent snapshot build for versions 1.0.1.BUILD-SNAPSHOT (spring-data-r2dbc-1.0.1.BUILD-20200123.090510-84.jar) and 1.1.0.BUILD-SNAPSHOT.

@Dav1dde
Copy link
Author

Dav1dde commented Jan 23, 2020

I can't test right now but the issue was even with changing the column name to the field name (or using the client directly):

client.select().from(Entity.class).matching(Criteria.where("id").in(List.of(existingUuid))).fetch().all()

returned 0 rows/entities, but changing Criteria.where("id").in(List.of(existingUuid)) to Criteria.where("id").is(existingUuid) did return a row/entity.

@IvanKonevJr
Copy link

@Dav1dde after pull it working, but need add: <artifactId>spring-data-commons</artifactId> with <version>2.3.0.BUILD-SNAPSHOT</version>

@Dav1dde
Copy link
Author

Dav1dde commented Jan 23, 2020

@LaoTsing thank you very much, to everyone ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-feedback We need additional information before we can continue type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants