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

DATACOUCH-179 : Customizing typeKeyValue when building a N1qlQuery #132

Closed

Conversation

Projects
None yet
6 participants
@zakariaeMahla
Copy link

commented Feb 5, 2017

No description provided.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Feb 5, 2017

@zakariaeMahla Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Feb 5, 2017

@zakariaeMahla Thank you for signing the Contributor License Agreement!

@zakariaeMahla zakariaeMahla changed the title #179 - Customizing typeKeyValue when building a N1qlQuery DATACOUCH-179 : Customizing typeKeyValue when building a N1qlQuery Feb 5, 2017

@bsubhashni
Copy link
Contributor

left a comment

Hi, Thanks much for contributing! It'd be great to get the comments addressed.

@@ -145,7 +146,7 @@ public static Expression createWhereFilterForEntity(Expression baseWhereCriteria
EntityMetadata<?> entityInformation) {
//add part that filters on type key
String typeKey = converter.getTypeKey();
String typeValue = entityInformation.getJavaType().getName();
String typeValue = getTypeValue(entityInformation);

This comment has been minimized.

Copy link
@bsubhashni

bsubhashni Feb 5, 2017

Contributor

This wouldn't really work, unless the documents are correctly serialized with the correct type value. Also, this covers only query derivation for n1ql. String based n1ql, spatial, view queries also use type mapping which are not addressed.

Type values are used in the Converter class to serialize entity to Couchbase Document and also in deserialization.

This comment has been minimized.

Copy link
@zakariaeMahla

zakariaeMahla Feb 5, 2017

Author

I didn't understand when you said : "unless the documents are correctly serialized with the correct type value"?
Actually, the method I have created covers the 2 cases :

  • When @typealias was used when serializing an object. It would be then logical to be also used when deserializing.
  • Else, we'll use the previous mapping, ie, the QFN of the class.

For the other String, Spatial and view queries, I'll check.

String real = N1qlUtils.createWhereFilterForEntity(null, converter, metadata).toString();

assertEquals(expected, real);
}

This comment has been minimized.

Copy link
@bsubhashni

bsubhashni Feb 5, 2017

Contributor

This particular change needs to be more integration tested. It is also great to have unit tests for more coverage.

This comment has been minimized.

Copy link
@zakariaeMahla

zakariaeMahla Feb 5, 2017

Author

I added this test for the if branch of my private method N1qlUtils.getTypeValue(EntityMetadata<?> entityInformation). The else branch has been already tested by the preexisting unit tests

@simonbasle

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

I'll defer to @subalakr on this one but the feature is definitely worth having, and direction taken for implementation seem rather correct.

Since string-based queries are constructed using N1qlUtils as well, it might be transparent for that part. Still please ensure this is working correctly with a simple string-based test that uses an annotated entity, @zakariaeMahla ;)

@bsubhashni

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

Thanks for the continued effort here @zakariaeMahla

When entities are saved through repository, they get converted to CouchbaseDocument using a MappingCouchbaseConverter which uses the type value here
https://github.com/spring-projects/spring-data-couchbase/blob/master/src/main/java/org/springframework/data/couchbase/core/convert/MappingCouchbaseConverter.java#L372

and reads also it. DefaultCouchbaseTypeMapper has a custom TypeAliasAccessor.
https://github.com/spring-projects/spring-data-couchbase/blob/master/src/main/java/org/springframework/data/couchbase/core/convert/MappingCouchbaseConverter.java#L302

https://github.com/spring-projects/spring-data-couchbase/blob/master/src/main/java/org/springframework/data/couchbase/core/convert/DefaultCouchbaseTypeMapper.java

The mapping information for a map function is quite a requirement for view/spatial queries to work correctly

String typeKey = couchbaseOperations.getConverter().getTypeKey();

An example of integration test is here, we would like to test against couchbase server and make sure the document can be queried through n1ql, view, spatial
https://github.com/spring-projects/spring-data-couchbase/blob/master/src/integration/java/org/springframework/data/couchbase/repository/Party.java
https://github.com/spring-projects/spring-data-couchbase/blob/master/src/integration/java/org/springframework/data/couchbase/repository/N1qlCouchbaseRepositoryTests.java

@MLabusquiere MLabusquiere referenced this pull request Sep 4, 2017

Closed

DATACOUCH-179 : with Integration Test #152

5 of 5 tasks complete
@MLabusquiere

This comment has been minimized.

Copy link

commented Sep 7, 2017

@davidkelly

This comment has been minimized.

Copy link
Collaborator

commented Jun 27, 2019

closing this as there are other PRs which address same

@davidkelly davidkelly closed this Jun 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.