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

Config constant inconsistency? #3

Open
leehericks opened this issue Jan 10, 2019 · 4 comments
Open

Config constant inconsistency? #3

leehericks opened this issue Jan 10, 2019 · 4 comments

Comments

@leehericks
Copy link

One quick one: I noticed TrustStrategy.trust_all_certificates and LoadBalancingStrategy::LEAST_CONNECTED seem to be inconsistent. Typo?

######################################
# Example 2.8. Trust
######################################

driver = Neo4j::Driver::GraphDatabase.driver(uri, Neo4j::Driver::AuthTokens.basic(user, password),
                                             trust_strategy: Neo4j::Driver::Config::TrustStrategy.trust_all_certificates)

######################################
# Example Load balancing strategy
######################################

driver = Neo4j::Driver::GraphDatabase.driver(uri, Neo4j::Driver::AuthTokens.basic(user, password),
                                             load_balancing_strategy: Neo4j::Driver::Config::LoadBalancingStrategy::LEAST_CONNECTED)
@klobuczek
Copy link
Member

To be honest this is inspired by Java. They have it the same way there. The reason is that there various TrustStrategies that have to be described with a class and the class constructed in some way. On the other hand, the LoadBalancingStrategy has only 2 simple options. Maybe it should better be
load_balancing_strategy: :least_connected
but not sure if that could become a limitation in the future.

@leehericks
Copy link
Author

@klobuczek I think it's ruby-like to do as you suggested but of course requires documentation to know the symbols.

load_balancing_strategy: :least_connected
load_balancing_strategy: :round_robin

What about this for trust strategy settings?
trust_strategy: :all
trust_strategy: { signed_by: cert_file }

@klobuczek
Copy link
Member

While the load_balancing_strategyis probably safe to convert to pure hash, TrustStrategy is a class in Java with some functionality behind it which I'm hesitant to already give up and model with nested hash. Maybe that will happen when implementing the MRI driver based on seabolt. I'll have to see the level of support for that over there. The goal is to keep identical API between jruby and seabolt based driver.

@leehericks
Copy link
Author

Ok, well I just thought it seemed inconsistent to have different programming strategies for the two options, especially when I saw how they defined TRUST_ALL_CERTIFICATES in the driver documentation. C#, Javascript, and Python all use enum or constants. Looks like they actually don't accept any cert file.

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

No branches or pull requests

2 participants