Skip to content

Verifies that the CassandraProperties default values are the same as driver built-in defaults #25130

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.

Sign up for GitHub

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
wants to merge 4 commits into from
Closed

Verifies that the CassandraProperties default values are the same as driver built-in defaults #25130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5f165bb
Verifies that the CassandraProperties default values are the same as …
onobc Feb 8, 2021
c416fca
Updates from PR review: Remove extraneous comments and assertJ failur…
onobc Feb 8, 2021
9eb10df
Updates from PR review: Remove extraneous comments and assertJ failur…
onobc Feb 8, 2021
36cd8a1
Removed unused import after removal of extra docs
onobc Feb 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...e/src/main/java/org/springframework/boot/autoconfigure/cassandra/CassandraProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,32 @@
import java.util.List;

import com.datastax.oss.driver.api.core.DefaultConsistencyLevel;
import com.datastax.oss.driver.api.core.config.OptionsMap;

import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.DeprecatedConfigurationProperty;

/**
* Configuration properties for Cassandra.
*
* <p>
* <strong>NOTE:</strong> default property values generally align with the Cassandra
* driver's {@link OptionsMap built-in defaults}.
*
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this. Whenever the team adds a property with a default value, they look for a test that checks default values and add an assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Yeh, I wanted to put thoughts there and then get guidance from team. I will remove.

* @author Julien Dubois
* @author Phillip Webb
* @author Mark Paluch
* @author Stephane Nicoll
* @author Chris Bono
* @since 1.3.0
*/
@ConfigurationProperties(prefix = "spring.data.cassandra")
public class CassandraProperties {

// NOTE: If you specify a default value for one of the driver properties be sure to
// add a test to CassandraPropertiesTest that verifies it is the same as the built-in
// driver's default value - unless the variance in value is intentional.

Copy link
Member

Choose a reason for hiding this comment

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

We don't add comments in code like that. Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

/**
* Keyspace name to use.
*/
Expand Down
42 changes: 42 additions & 0 deletions ...c/test/java/org/springframework/boot/autoconfigure/cassandra/CassandraPropertiesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.springframework.boot.autoconfigure.cassandra;

import com.datastax.oss.driver.api.core.config.OptionsMap;
import com.datastax.oss.driver.api.core.config.TypedDriverOption;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

/**
* Tests for {@link CassandraProperties}.
*
* @author Chris Bono
*/
class CassandraPropertiesTest {

@Test
void defaultValuesAreConsistent() {
String failMsg = "the default value has diverged from the driver's built-in default";
Copy link
Member

Choose a reason for hiding this comment

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

While I can't deny it makes the assertions nicer, we're not doing there anywhere else. If you want to discuss the merit of adding such a message, I am happy to consider it but it should be part of a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would only be valuable in an ecosystem where the team was not aware of defaults and what that all entails (adding an entry to this test, etc). I would definitely add this in my codebase at work but as you stated above, its not needed here. Everyone is "in the know". Removing..


CassandraProperties properties = new CassandraProperties();
OptionsMap optionsMap = OptionsMap.driverDefaults();

assertThat(properties.getConnection().getConnectTimeout()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.CONNECTION_CONNECT_TIMEOUT));

assertThat(properties.getConnection().getInitQueryTimeout()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.CONNECTION_INIT_QUERY_TIMEOUT));

assertThat(properties.getRequest().getTimeout()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.REQUEST_TIMEOUT));

assertThat(properties.getRequest().getPageSize()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.REQUEST_PAGE_SIZE));

assertThat(properties.getRequest().getThrottler().getType().type()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.REQUEST_THROTTLER_CLASS));

assertThat(properties.getPool().getHeartbeatInterval()).describedAs(failMsg)
.isEqualTo(optionsMap.get(TypedDriverOption.HEARTBEAT_INTERVAL));
}

}