[
https://issues.apache.org/jira/browse/CASSANDRA-20404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17936673#comment-17936673
]
Maulin Vasavada edited comment on CASSANDRA-20404 at 3/19/25 7:48 PM:
----------------------------------------------------------------------
[~Jyothsnakonisa] you can take a look as well. I am particularly concerned
about a previous code that ONLY used `max_certificate_validity_period` for both
the fields - for max cert validity period AND
`certificate_validity_warn_threshold` as per [these
lines|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L915-L1179]
I ran most relevant tests after my changes and didn't find any issues but you
can double check if the new code is doing the right thing. From the older code
I feel it may as well have a typo actually but could not be sure.
Sample code from the tunk-
{code:java}
public ServerEncryptionOptions withOutboundKeystorePasswordFile(String
outboundKeystorePasswordFile)
{ return new ServerEncryptionOptions(ssl_context_factory, keystore,
keystore_password, keystore_password_file,
outbound_keystore, outbound_keystore_password,
outboundKeystorePasswordFile,
truststore, truststore_password, truststore_password_file,
cipher_suites, protocol, accepted_protocols,
algorithm, store_type,
require_client_auth,
require_endpoint_verification, optional, internode_encryption,
legacy_ssl_storage_port_enabled,
max_certificate_validity_period,
max_certificate_validity_period).applyConfigInternal(); }
{code}
was (Author: maulin.vasavada):
[~Jyothsnakonisa] you can take a look as well. I am particularly concerned
about a previous code that ONLY used `max_certificate_validatity_period` for
both the fields - for max cert validity period AND
`certifcate_validity_warn_threshold` as per [these
lines|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L915-L1179]
I ran most relevant tests after my changes and didn't find any issues but you
can double check if the new code is doing the right thing. From the older code
I feel it may as well have a typo actually but could not be sure.
Sample code from the tunk-
{code:java}
public ServerEncryptionOptions withOutboundKeystorePasswordFile(String
outboundKeystorePasswordFile)
{ return new ServerEncryptionOptions(ssl_context_factory, keystore,
keystore_password, keystore_password_file,
outbound_keystore, outbound_keystore_password,
outboundKeystorePasswordFile,
truststore, truststore_password, truststore_password_file,
cipher_suites, protocol, accepted_protocols,
algorithm, store_type,
require_client_auth,
require_endpoint_verification, optional, internode_encryption,
legacy_ssl_storage_port_enabled,
max_certificate_validity_period,
max_certificate_validity_period).applyConfigInternal(); }
{code}
> Improve EncryptionOptions class's `with` methods using Builder pattern
> ----------------------------------------------------------------------
>
> Key: CASSANDRA-20404
> URL: https://issues.apache.org/jira/browse/CASSANDRA-20404
> Project: Apache Cassandra
> Issue Type: Improvement
> Reporter: Maulin Vasavada
> Assignee: Maulin Vasavada
> Priority: Normal
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently,
> [EncryptionOptions.java|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L520]
> allows constructing the object using `with` prefix method pattern which
> sounds similar to the Builder Pattern
> ([example)|https://blogs.oracle.com/javamagazine/post/exploring-joshua-blochs-builder-design-pattern-in-java]
> However, when there is any new parameter introduced in the class for a
> corresponding configuration option, it requires changes to all the
> constructor calls from those `with` methods to reflect the new parameter.
> This is error prone given so many string parameters and considering some
> [special
> use-cases|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L1190]
> it makes it challenging to modify the code for a simple task - adding a new
> parameter in the class.
> Proposal here is to use the Builder pattern (like shows in the example linked
> above) to improve the code in EncryptionOptions.java file.
> There could be a potential to consider using "java record" type that got
> introduced in Java 14+ or using [Lombok plugin|https://projectlombok.org/]
> but in order to be backward compatible and avoid additional dependency on
> external packages- may be we can simply follow the builder pattern.
> One change that we have to be careful about is - the new builder pattern
> implementation may not call `applyConfig()` everytime upon calling `with`
> methods compared to the current implementation. Only when final
> `Builder#build()` happens it will call the applyConfig() method . While that
> saves multiple initialization that happen today in Tests due to heavy usage,
> we have to assess if there is any impact due to this behavior change.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
