neo4jrb / neo4j-ruby-driver

Neo4j Ruby Driver
MIT License
39 stars 29 forks source link

Config constant inconsistency? #3

Open leehericks opened 5 years ago

leehericks commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.