Skip to content

Improve framework for custom mapping couchbase converter. #1148

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

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

mikereiche
Copy link
Collaborator

@mikereiche mikereiche commented Jun 8, 2021

  1. change examples in Config to show creating repository mappings using
    existing MappingCouchbaseConverter (which have customConversions /
    BeanNames.COUCHBASE_CUSTOM_CONVERSIONS and applicationContext)
    instead of creating their own and then adding the customConversion and
    applicationContext.

  2. remove unused members fro StringBasedN1qlQueryParser

Closes #1141.

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@mikereiche mikereiche requested a review from daschl June 8, 2021 19:30
@mikereiche mikereiche force-pushed the datacouch_1141_better_custom_mapping branch 2 times, most recently from ec1e313 to 8be47bf Compare June 8, 2021 20:31
Copy link
Contributor

@daschl daschl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+2, just one minor comment that can slightly improve user experience. but feel free to merge

@@ -148,6 +148,8 @@ public MappingCouchbaseConverter(
this.mappingContext = mappingContext;
typeMapper = new DefaultCouchbaseTypeMapper(typeKey != null ? typeKey : TYPEKEY_DEFAULT);
spELContext = new SpELContext(CouchbaseDocumentPropertyAccessor.INSTANCE);
// ApplicationContext is needed for environment for resolving SPEL expressions.
this.setApplicationContext(((CouchbaseMappingContext) mappingContext).getApplicationContext());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we throw an exception if the mappingContext is not instanceof CouchbaseMappingContext? So it will be descriptive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the converter should be ApplicationContextAware so that the bean container injects the context. You could skip that line and stop exposing the context from the mapping context for a clean implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This shouldn't be done here. It's similar to afterPropertiesSet() call, in that it should normally be done by the Bean container. And if the object is created outside the bean container, then the caller has to take care of the initialization that the bean container would normally do. So I'll remove this and not expose the applicationContext of the mapping context.

@@ -74,6 +73,7 @@ public ConversionService getConversionService() {
*/
public void setCustomConversions(final CustomConversions conversions) {
this.conversions = conversions;
afterPropertiesSet(); // register the conversions with the conversion service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refrain from calling lifecycle methods in setters. Calling afterPropertiesSet is a responsibility of the bean container.

Copy link
Collaborator Author

@mikereiche mikereiche Jun 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can fix that by instead calling conversions.registerConvertersIn(conversionService) directly here.

The underlying issue is that
(a) setCustomConversions() doesn't do everything needed to setCustomConversions(). It only does this.conversions = conversions and that isn't sufficient. It is also necessary to conversions.registerConvertersIn(conversionService) - otherwise the conversionService is not aware of the conversions. I made the call to afterPropertiesSet() in the event that in the future additional actions were added.
(b) the mapping converter was created outside the Bean container - in #1141 - although that should be unnecessary by creating the template for configureRepositoryOperationsMapping using the converter (MappingCouchbaseConverter) (baseMapping.getDefault().getConverter()) .

Somewhat related - Intellij complains when an @bean is called directly, but sometimes it is necessary - when the value of an @bean method is being used in another public @bean method, and the signature of that public @bean method cannot (should not?) be changed. On the other hand - if there was a guarantee that no applications were calling the @bean method directly, then changing the signature should not be a problem. This issue exists in three bean methods in AbstractCouchbaseConfiguration. The "solution" has been to make @bean method call - the problem with this is that the result of that call is not the actual @bean, it's a different object. (I suppose that if the result object was cached and used on subsequent calls, then it could be the same object).

couchbaseObjectMapper() is an @bean method, it is now required by the couchbaseClusterEnvironment() @bean, but was not/is not a method argument. This had been "fixed" previously by having an @Autowired CouchbaseObjectMapper in the class - but that resulted in a circular reference when @componentscan was used.

	@Bean(destroyMethod = "shutdown")
	public ClusterEnvironment couchbaseClusterEnvironment() 
	   ... 
	   builder.jsonSerializer(JacksonJsonSerializer.create(couchbaseObjectMapper()));

This couchbaseTemplate() method was previously the @bean, but has been superseded with a method with an additional argument.

	public CouchbaseTemplate couchbaseTemplate(CouchbaseClientFactory couchbaseClientFactory,
			MappingCouchbaseConverter mappingCouchbaseConverter) {
		return couchbaseTemplate(couchbaseClientFactory, mappingCouchbaseConverter, new JacksonTranslationService());
	}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setCustomConversions() doesn't do everything needed to setCustomConversions(). It only doe

That's by intent. The bean configuration phase consists from calling all setters first and then initialize the bean with afterPropertiesSet. That being said, calling registerConvertersIn(…) in the setter isn't better.

the mapping converter was created outside the Bean container

It is then the responsibility of the creator to initialize that bean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I'll just remove the call and not replace it with anything.

1) change examples in Config to show creating repository mappings using
existing MappingCouchbaseConverter (which have customConversions /
BeanNames.COUCHBASE_CUSTOM_CONVERSIONS and applicationContext)
instead of creating their own and then adding the customConversion and
applicationContext.

2) replace @Autowired couchbaseObjectMapper with couchbaseObjectMapper()
to avoid the following when using @componentscan :
Unsatisfied dependency expressed through field 'couchbaseObjectMapper';
nested exception is org.springframework.beans.factory.
BeanCurrentlyInCreationException: Error creating bean with name
'couchbaseObjectMapper': Requested bean is currently in creation: Is
there an unresolvable circular reference?

3) remove unused members fro StringBasedN1qlQueryParser

Closes #1141.
@mikereiche mikereiche force-pushed the datacouch_1141_better_custom_mapping branch from 8be47bf to a3d1730 Compare June 10, 2021 01:29
@mikereiche mikereiche merged commit 67a7937 into main Jun 10, 2021
mikereiche added a commit that referenced this pull request Aug 9, 2021
1) change examples in Config to show creating repository mappings using
existing MappingCouchbaseConverter (which have customConversions /
BeanNames.COUCHBASE_CUSTOM_CONVERSIONS and applicationContext)
instead of creating their own and then adding the customConversion and
applicationContext.

2) replace @Autowired couchbaseObjectMapper with couchbaseObjectMapper()
to avoid the following when using @componentscan :
Unsatisfied dependency expressed through field 'couchbaseObjectMapper';
nested exception is org.springframework.beans.factory.
BeanCurrentlyInCreationException: Error creating bean with name
'couchbaseObjectMapper': Requested bean is currently in creation: Is
there an unresolvable circular reference?

3) remove unused members fro StringBasedN1qlQueryParser

Closes #1141.

Co-authored-by: mikereiche <[email protected]>
@mikereiche mikereiche deleted the datacouch_1141_better_custom_mapping branch August 11, 2021 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom converter for multibuckets
3 participants