Skip to content

Enums can't be used as method parameters in Repository #1069

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
edudar opened this issue Jan 29, 2021 · 13 comments · Fixed by #1111 or #1112
Closed

Enums can't be used as method parameters in Repository #1069

edudar opened this issue Jan 29, 2021 · 13 comments · Fixed by #1111 or #1112
Labels
status: waiting-for-triage An issue we've not yet triaged

Comments

@edudar
Copy link

edudar commented Jan 29, 2021

When I have the following classes:

enum Status { A,B }

@Document
class Entity { @Field Status status; }

I can write and read entities from the DB with no issues. However, when I create a function that would search for entities using status, it fails with

com.couchbase.client.core.error.InvalidArgumentException: Unsupported type for JSON value: class Status
	at com.couchbase.client.java.json.JsonValue.coerce(JsonValue.java:94)
	at com.couchbase.client.java.json.JsonArray.add(JsonArray.java:178)
	at org.springframework.data.couchbase.core.query.QueryCriteria.maybeWrapValue(QueryCriteria.java:387)

Looking into the code, I see that QueryCriteria tries to convert Enum using AbstractCouchbaseConverter.convertForWriteIfNeeded. There it first looks converter up in CouchbaseCustomConversions but can't find it as conversions know about their own and parent converters only.

converters.addAll(DateConverters.getConvertersToRegister());
converters.addAll(CouchbaseJsr310Converters.getConvertersToRegister());

defaults.addAll(JodaTimeConverters.getConvertersToRegister());
defaults.addAll(Jsr310Converters.getConvertersToRegister());
defaults.addAll(ThreeTenBackPortConverters.getConvertersToRegister());

However, the actual conversion would be performed, if conversion found, by conversionService which is DefaultConversionService and this guy actually knows how to convert to and from Enum.

Adding custom implementations into CouchbaseCustomConversions solves this but I feel like it should not be necessary considering DefaultConversionService can do this already.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 29, 2021
@mikereiche
Copy link
Collaborator

Thanks @edudar. I'll look at this when I get a chance.

mikereiche added a commit that referenced this issue Mar 24, 2021
Support enums in AbstractCouchbaseConverter.convertForWriteIfNeeded().

Closes #1069.
Original pull request #1110.
mikereiche added a commit that referenced this issue Mar 24, 2021
Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded()
and also call that from
MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite()

Closes #1069.
Original pull request #1111.
mikereiche added a commit that referenced this issue Mar 24, 2021
Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded()
and also call that from
MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite()

Closes #1069.
Original pull request #1112.

Co-authored-by: mikereiche <[email protected]>
mikereiche added a commit that referenced this issue Mar 25, 2021
* Add QueryCriteria arrayContaining.

Add QueryCriteria arrayContaining which maps to n1ql array_containing.

Closes #1073.
Original pull request #1109.

* Support enum parameters to repository queries.

Support enums in AbstractCouchbaseConverter.convertForWriteIfNeeded().

Closes #1069.
Original pull request #1110.

Co-authored-by: mikereiche <[email protected]>
mikereiche added a commit that referenced this issue Mar 30, 2021
Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded()
and also call that from
MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite()

Closes #1069.
Original pull request #1112.

Co-authored-by: mikereiche <[email protected]>
mikereiche added a commit that referenced this issue Mar 30, 2021
Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded()
and also call that from
MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite()

Closes #1069.
Original pull request: #1112.

Co-authored-by: mikereiche <[email protected]>
mikereiche added a commit that referenced this issue Mar 30, 2021
Support enum in AbstractCouchbaseConverter.convertForWriteIfNeeded()
and also call that from
MappingCouchbaseConverter.getPotentiallyConvertedSimpleWrite()

Closes #1069.
Original pull request: #1112.

Co-authored-by: mikereiche <[email protected]>
@mstfymrtc
Copy link

Any progress on this? :/

@mikereiche
Copy link
Collaborator

It's in 4.2.0

d16bbff

Also - it seems that using .toString() of the enum should work as well.

@mstfymrtc
Copy link

It's in 4.2.0

d16bbff

Also - it seems that using .toString() of the enum should work as well.

Yeah, it's a workaround i've used too. Thanks for the contribution <3

@vforchi
Copy link

vforchi commented Nov 11, 2021

I ran into the same problem, I upgraded to 4.2 but the problem is still there. I believe the reason is that I am using a List of enums, and this won't work

return this.conversions.getCustomWriteTarget(value.getClass()) //
				.map(it -> (Object) this.conversionService.convert(value, it)) //
				.orElseGet(() -> Enum.class.isAssignableFrom(value.getClass()) ? ((Enum<?>) value).name() : value);

@mikereiche
Copy link
Collaborator

@vforchi

Collections and Arrays are handled by the calling code (and produce a CouchbaseList). Can you show what issue you have? Maybe it is somewhere other than the converter?

	if (elementType == null || conversions.isSimpleType(elementType)) {
			target.put(getPotentiallyConvertedSimpleWrite(element));
		} else if (element instanceof Collection || elementType.isArray()) {
			target.put(writeCollectionInternal(asCollection(element), new CouchbaseList(conversions.getSimpleTypeHolder()),
					componentType));
		} else {

@mikereiche
Copy link
Collaborator

mikereiche commented Nov 12, 2021

This is with 4.3-RC1, but 4.2.0 should have the same handling of an array of Enum

I add this to AirportRepository of the unit tests.

@ScanConsistency(query = QueryScanConsistency.REQUEST_PLUS)
Airport findByIataIn(Iata[] iata);

I add this to CouchbaseRepositoryQueryIntegrationTests.findByEnum()

Airport airport3 = airportRepository.findByIataIn( new Iata[]{Iata.vie, Iata.xxx});
System.out.println("airport3: "+airport3);
assertNotNull(airport3, "should have found " + vie);
assertEquals(airport3.getId(), vie.getId());

airport3: {"id":"airports::vie","iata":"vie","icao":"loww","version":1636687747381460992,"createdBy":"nonreactive_auditor","expiration":0,"size":0,"someNumber":0}

It produces the n1ql:

SELECT META(my_bucket).id AS __id, META(my_bucket).cas AS __cas, iata, icao, createdBy, META(my_bucket).expiration, size, someNumberFROMmy_bucketWHERE_class= "airport" ANDiata in ( $1 )

@vforchi
Copy link

vforchi commented Nov 12, 2021

I will check, but I am using a collection, not an array.

@mikereiche
Copy link
Collaborator

The problem is reproducible with a Collection. Fixing.

@mikereiche
Copy link
Collaborator

Making a new issue to handle collection arguments. #1270

@Pharisaeus
Copy link

Pharisaeus commented Oct 5, 2023

@mikereiche I hit the same issue when using a custom @Query and named @Param as in:

@Query("UPDATE #{#n1ql.collection} SET status = $newStatus")
Result changeStatus(@Param("newStatus") Status newStatus);

gives:

com.couchbase.client.core.error.InvalidArgumentException: Unsupported type for JSON value: class a.b.c.Status
	at com.couchbase.client.core.error.InvalidArgumentException.fromMessage(InvalidArgumentException.java:28) ~[core-io-2.4.10.jar:na]
	at com.couchbase.client.java.json.JsonValue.coerce(JsonValue.java:94) ~[java-client-3.4.10.jar:na]
	at com.couchbase.client.java.json.JsonObject.put(JsonObject.java:222) ~[java-client-3.4.10.jar:na]
	at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.putNamedValue(StringBasedN1qlQueryParser.java:575) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getNamedPlaceholderValues(StringBasedN1qlQueryParser.java:517) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getPlaceholderValues(StringBasedN1qlQueryParser.java:543) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.core.query.StringQuery.toN1qlSelectString(StringQuery.java:83) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.core.ReactiveFindByQueryOperationSupport$ReactiveFindByQuerySupport.assembleEntityQuery(ReactiveFindByQueryOperationSupport.java:281) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.core.ReactiveFindByQueryOperationSupport$ReactiveFindByQuerySupport.all(ReactiveFindByQueryOperationSupport.java:182) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.core.ExecutableFindByQueryOperationSupport$ExecutableFindByQuerySupport.all(ExecutableFindByQueryOperationSupport.java:95) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQuery.lambda$getExecutionToWrap$1(AbstractCouchbaseQuery.java:124) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.CouchbaseQueryExecution$ResultProcessingExecution.execute(CouchbaseQueryExecution.java:84) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQuery.doExecute(AbstractCouchbaseQuery.java:93) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQueryBase.execute(AbstractCouchbaseQueryBase.java:132) ~[spring-data-couchbase-5.1.4.jar:5.1.4]
	at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQueryBase.execute(AbstractCouchbaseQueryBase.java:113) ~[spring-data-couchbase-5.1.4.jar:5.1.4]

I'm guessing it goes through a different code path in this scenario and the previous fix is not enough.

@mikereiche
Copy link
Collaborator

hi @Pharisaeus - Thanks for bring this to our attention. I opened a new issue and tagged you in it. #1837.
The same converter is supposed to be used for Parameters, but apparently it is not in this case.

@jakov222
Copy link

hey, looks like the query methods using collections of enum as a paremeter are still not working correctly. I created another issue #1855
tagging @mikereiche

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
7 participants