DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport. #294

jorgerod · 2020-12-14T16:26:37Z

TranslationService is defined as bean but in CouchbaseTemplateSupport.
That bean is not being used and a new instance of JacksonTranslationSerivce is being created in the constructor.

We want CouchbaseTemplateSupport to receive a TranslationService type bean.

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).

pivotal-issuemaster · 2020-12-14T16:26:39Z

@jorgerod 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 · 2020-12-14T16:27:12Z

@jorgerod Thank you for signing the Contributor License Agreement!

mikereiche · 2020-12-16T01:55:25Z

This looks good to me. I'm adding another reviewer.

jorgerod · 2020-12-16T08:19:27Z

Hi @mikereiche @daschl

I think it would be interesting to make a backport to 4.1.x.

How do you see it?

daschl · 2020-12-16T13:00:38Z

src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java

@@ -145,14 +146,14 @@ protected void configureEnvironment(final ClusterEnvironment.Builder builder) {

 	@Bean(name = BeanNames.COUCHBASE_TEMPLATE)
 	public CouchbaseTemplate couchbaseTemplate(CouchbaseClientFactory couchbaseClientFactory,
-			MappingCouchbaseConverter mappingCouchbaseConverter) {
-		return new CouchbaseTemplate(couchbaseClientFactory, mappingCouchbaseConverter);
+			MappingCouchbaseConverter mappingCouchbaseConverter, TranslationService translationService) {


this breaks backwards compatibility by introducing another required argument - if this should be added we need another overload that keeps the defaults as is I think

Hi @daschl

You are right about the overloading of the CouchbaseTemplate and ReactiveCouchbaseTemplate constructors. I'll update it now.

As for the definition of beans couchbaseTemplate and reactiveCouchbaseTemplate, do you consider any change necessary? I didn't quite understand you.

Yeah, I was actually also talking about the bean definitions - because if someone overloads them in their actual configuration, just changing the signature on update will fail to compile for them.

Hi @daschl

It's true that is a breaking change, but I can't think of any other way to do it. Bean couchbaseTranslateService is being created but then not used.

If you don't want to be breakers, we could keep the overload on the CouchbaseTemplate and ReactiveCouchbaseTemplate constructors and at another time add the TranslationService parameter to the configuration beans and then be breakers.

What do you think?

Thank you very much

@jorgerod well, you could make the beans take the new parameter by default (as you do now), but add another 2 methods that look like the old signature and not be a Bean, which call into the new beans and just initialize the json translation service. Then if someone overrides it, their code would not break but everyone can then switch over to the new bean if needed?

I understand. Thank you very much for the help @daschl

daschl

I think this contains a breaking change as-is

mikereiche · 2020-12-16T17:24:49Z

src/main/java/org/springframework/data/couchbase/config/AbstractCouchbaseConfiguration.java

@@ -145,14 +146,14 @@ protected void configureEnvironment(final ClusterEnvironment.Builder builder) {

 	@Bean(name = BeanNames.COUCHBASE_TEMPLATE)
 	public CouchbaseTemplate couchbaseTemplate(CouchbaseClientFactory couchbaseClientFactory,
-			MappingCouchbaseConverter mappingCouchbaseConverter) {
-		return new CouchbaseTemplate(couchbaseClientFactory, mappingCouchbaseConverter);
+			MappingCouchbaseConverter mappingCouchbaseConverter, TranslationService translationService) {


The translationService argument should be named couchbaseTranslationService to match the bean name.
When the argument name does not match the bean name, the spring framework can only match by the type, and if there is ever another bean of type TranslationService added, the spring framework will not be able to automatically match this argument with the couchbaseTranslationService bean.

mikereiche · 2020-12-17T19:21:21Z

@jorgerod - Please put a period at the end of the first line of the commit message. (they are parsed programmatically) Thanks.

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport

jorgerod · 2020-12-18T08:12:05Z

@jorgerod - Please put a period at the end of the first line of the commit message. (they are parsed programmatically) Thanks.

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport

Hi @mikereiche

I've checked the commit messages and they all have a period at the end of the line.

jorgerod · 2020-12-22T08:48:41Z

Hello @daschl @mikereiche

Have you been able to see the latest updates?

Greetings

mikereiche · 2021-01-14T20:59:38Z

@jorgerod - can you resolve the conflicts? Then I will merge this.

jorgerod · 2021-01-15T09:16:42Z

@mikereiche Conflicts resolved.

mp911de · 2021-01-15T09:50:55Z

@jorgerod please squash your commits into a single one as we keep a single commit (and a linear history) per change.

…upport.

jorgerod · 2021-01-15T11:42:09Z

Hi @mp911de

I have already squashes my commits. Does it look good?

Thank you

jorgerod · 2021-01-18T08:12:22Z

Hi @mikereiche @daschl

I think it would be interesting to make a backport to 4.1.x.

How do you see it?

Hi @mikereiche

How does it look?

mikereiche · 2021-01-18T17:00:29Z

It will be in the next 4.1.x release scheduled for February 10.

…upport. (#294)

mikereiche requested review from daschl and mikereiche December 16, 2020 01:54

mikereiche changed the title ~~DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSuppot~~ DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport Dec 16, 2020

daschl reviewed Dec 16, 2020

View reviewed changes

daschl suggested changes Dec 16, 2020

View reviewed changes

mikereiche reviewed Dec 16, 2020

View reviewed changes

jorgerod changed the title ~~DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport~~ DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport. Dec 18, 2020

spring-projects-issues mentioned this pull request Dec 31, 2020

CouchbaseTemplateSupport does not allow a TranslationService bean to be injected [DATACOUCH-652] #962

Closed

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 4, 2021

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateS…

074b454

…upport.

jorgerod force-pushed the datacouch_652_inject_bean_translator_service_to_couchbase_template_support branch from 51ca98a to 074b454 Compare January 15, 2021 11:38

mikereiche merged commit 1329cae into spring-projects:master Jan 15, 2021

jorgerod deleted the datacouch_652_inject_bean_translator_service_to_couchbase_template_support branch January 18, 2021 08:12

mikereiche pushed a commit that referenced this pull request Feb 16, 2021

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateS…

c2741d8

…upport. (#294)

jorgerod mentioned this pull request May 25, 2021

Feature 'Inject translation service bean in CouchbaseTemplateSupport' has been deleted #1140

Closed

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport. #294

DATACOUCH-652 - Inject translation service bean in CouchbaseTemplateSupport. #294

Uh oh!

Conversation

jorgerod commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pivotal-issuemaster commented Dec 14, 2020

Uh oh!

pivotal-issuemaster commented Dec 14, 2020

Uh oh!

mikereiche commented Dec 16, 2020

Uh oh!

jorgerod commented Dec 16, 2020

Uh oh!

daschl Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

jorgerod Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

daschl Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

jorgerod Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

daschl Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

jorgerod Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

daschl left a comment

Choose a reason for hiding this comment

Uh oh!

mikereiche Dec 16, 2020

Choose a reason for hiding this comment

Uh oh!

mikereiche commented Dec 17, 2020

Uh oh!

jorgerod commented Dec 18, 2020

Uh oh!

jorgerod commented Dec 22, 2020

Uh oh!

mikereiche commented Jan 14, 2021

Uh oh!

jorgerod commented Jan 15, 2021

Uh oh!

mp911de commented Jan 15, 2021

Uh oh!

jorgerod commented Jan 15, 2021

Uh oh!

jorgerod commented Jan 18, 2021

Uh oh!

mikereiche commented Jan 18, 2021

Uh oh!

Uh oh!

jorgerod commented Dec 14, 2020 •

edited

Loading