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

CassandraAutoconfiguration leads to ignored parameters in db-config.conf #31025

Closed
nilsvillwock opened this issue May 13, 2022 · 8 comments
Closed
Labels
status: superseded An issue that has been superseded by another

Comments

@nilsvillwock
Copy link

nilsvillwock commented May 13, 2022

We are using the cassandra driver specific configuration file db-config.conf via the property:

spring:
  data:
    cassandra:
      config: file:/db-config.conf

The problem now is that even when I do not define further spring-boot CassandraProperties in the yaml, there are default values already set and these properties are ignored in the db-config.conf. file. The reason seem to be, because the db-config.conf properties are loaded as fallback after the default spring boot CassandraProperties. Spring Boot CassandraProperties should win, but only if they are actively set.

The following Cassandra properties always initialized and so always ignored in db-config.conf

initCassandraProperties

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 13, 2022
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2022
@philwebb philwebb added this to the 2.5.x milestone May 13, 2022
@snicoll
Copy link
Member

snicoll commented May 16, 2022

@philwebb I can see you've flagged this as a bug but the current behavior is the expected behavior, see https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#data.nosql.cassandra.connecting. Perhaps the documentation should be more explicit that a default value counts as a value being present.

@nilsvillwock the purpose of the config property is to let define properties that the Spring Boot namespace does not support.

@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review and removed for: team-attention An issue we'd like other members of the team to review labels May 16, 2022
@nilsvillwock
Copy link
Author

OK, please adjust the documentation from:

If a property is both present in spring.data.cassandra.* and the configuration file, the value in spring.data.cassandra.* takes precedence.

to:

If a property is both present (even it is only by default) in spring.data.cassandra.* and the configuration file, the value in spring.data.cassandra.* takes precedence.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label May 16, 2022
@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label May 18, 2022
@wilkinsona wilkinsona modified the milestones: 2.5.x, 2.6.x May 18, 2022
@snicoll
Copy link
Member

snicoll commented May 18, 2022

We've discussed this at the team meeting and we come to realize that it was supposed to work as expected (see #24065). The problem is that some properties were overlooked, typically the ones that have a primitive type (and a default value of 0).

@nilsvillwock it would be good to list the properties that you tried to define in db-config.conf that did not work.

@nilsvillwock
Copy link
Author

@snicoll
The properties which are ignored in the db-config file are the ones you can see at the screenshot in my initial post

ittays pushed a commit to ittays/spring-boot that referenced this issue Jun 1, 2022
…ride defaults

This commit changes two things:

1. Any primitive on CassandraProperties are replaced with object values.
   This allows distinguishing between defaults values and no-values. Then
   CassandraAutoConfiguration.mapConfig() can use whenNonNull() predicate
   to ignore those.

2. CassandraProperties no longer populate default values on any
   property. With that, the defaults can be applied on top of the file
   spring.data.cassandra.config; i.e. the config file have higher
   precedence than the defaults, but lower that any spring.data.cassandra.*
   property.

Closes spring-projectsgh-31025
ittays pushed a commit to ittays/spring-boot that referenced this issue Jun 1, 2022
This commit changes two things:

1. Any primitive on CassandraProperties are replaced with object values.
   This allows distinguishing between defaults values and no-values. Then
   CassandraAutoConfiguration.mapConfig() can use whenNonNull() predicate
   to ignore those.

2. CassandraProperties no longer populate default values on any
   property. With that, the defaults can be applied on top of the file
   spring.data.cassandra.config; i.e. the config file have higher
   precedence than the defaults, but lower that any spring.data.cassandra.*
   property.

Closes spring-projectsgh-31025
ittays pushed a commit to ittays/spring-boot that referenced this issue Jun 1, 2022
This commit changes two things:

1. Any primitive on CassandraProperties are replaced with object values.
   This allows distinguishing between defaults values and no-values. Then
   CassandraAutoConfiguration.mapConfig() can use whenNonNull() predicate
   to ignore those.

2. CassandraProperties no longer populate default values on any
   property. With that, the defaults can be applied on top of the file
   spring.data.cassandra.config; i.e. the config file have higher
   precedence than the defaults, but lower that any spring.data.cassandra.*
   property.

Closes spring-projectsgh-31025
ittays pushed a commit to ittays/spring-boot that referenced this issue Jun 1, 2022
This commit changes two things:

1. Any primitive on CassandraProperties are replaced with object values.
   This allows distinguishing between defaults values and no-values. Then
   CassandraAutoConfiguration.mapConfig() can use whenNonNull() predicate
   to ignore those.

2. CassandraProperties no longer populate default values on any
   property. With that, the defaults can be applied on top of the file
   spring.data.cassandra.config; i.e. the config file have higher
   precedence than the defaults, but lower that any spring.data.cassandra.*
   property.

On the test side, the file `override-defaults.conf` is defined to override
the values that were non-overridable with values which are different from
the default.

Closes spring-projectsgh-31025
@ittays
Copy link

ittays commented Jun 1, 2022

I looked into the issue, and started to write a solution. My general approach is here: ittays@f09e2b3

As I wrote there on the commit message, there are two changes required: one is removing primitives from CassandraProperties in favor of nullable-values.
The second is splitting the spring.data.cassandra.* from the hard-coded values, as the latter has lower precedence.

It is still not complete, as introducing the nullable CassandraProperties fields requires adding null-handling flows (especially for port and ssl).

In addition, I'm not sure about where to locate the separated values. I defined a defaults() static method on CassandraProperties, but his doesn't feel right.


Last but not least, I found a significant flow which requires an additional thought: when contact-points values are configured without a port, but an actual port is defined on a different config layer. What is the expected behavior?

Consider both cases when, say, system properties define the port and cassandra.config defines the host, and vice versa. What should we expect to happen? Should the port from one config layer be considered along with the port from the other (higher/lower) layer?

Today the host:port construction happens on the layer-level. I think a better approach is do defer the construction to the point where all layers are combined already.

@ittays
Copy link

ittays commented Jun 2, 2022

Can you please look into my PR?
Eventually, to simplify the port configuration, I preferred not to defer the contact-points config, and not to cross-use a host from one config layer and a port from another.

@snicoll
Copy link
Member

snicoll commented Jun 2, 2022

@ittays there's no need to ask us to do that. The whole team already received a notification when you created the PR.

@philwebb
Copy link
Member

philwebb commented Jun 9, 2022

Closing in favor of PR #31238. Thanks @ittays!

@philwebb philwebb closed this as completed Jun 9, 2022
@philwebb philwebb added status: superseded An issue that has been superseded by another and removed type: bug A general bug labels Jun 9, 2022
@philwebb philwebb removed this from the 2.6.x milestone Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
6 participants