search

ravendb / ravendb-jvm-client

RavenDB JVM Client
MIT License
13 stars 13 forks source link

Add a DocumentStore.Builder #66

Closed cmitchell closed 2 months ago

cmitchell commented 1 year ago

Hi,

Would you be amenable to modifying the DocumentStore to use the Builder pattern? It has several advantages. The biggest are that it's self documenting (quite a few of the method javadocs seem to be missing) and that business logic of what can be called before certain points in time vs later are hidden from the user. The user would know what the absolutely required params are, i.e. just the url and database (which is checked for in the assertValidConfiguration I believe) because they are required in the constructor. That would also make it clear that certain methods must be called at "building" time, i.e. before initialize since after some deprecation period setters would only be available through the builder. In fact, initialize() would be called from the build() so the user wouldn't need to know. As it is now, users have to quasi understand your business logic flow instead of just needing to know how to configure the DocumentStore once and from then on use it to get sessions. (Also, internally you could stop relying on assertions that the store has been initialized because the only way to get a store ti through the builder, which would initialize it) In short, I'm proposing adding to the code so that a DocumentStore is built (and initialized) like this

    DocumentStore store = new DocumentStore.Builder(urls, database)
         .setCertficiate(cert)
         .setPrivateKeyPassword(password)
         .setTruststore(truststore
        .setConventions(new DocumentConventions.Builder().setUseOptomisticLocking(true).build())
        .build();

This would return a DocumentStore ready to call openSession (i.e. already initalized). This pattern wold be more intuitive to jvm language programmers, as it's what's used for mongo and others.

I would be willing to do the work (addition of code and tests) and submit a PR, but wanted to know if it would be a welcome addition. In the long term, I wouldn't remove the existing access to the setters and constructors, but I would mark them as Deprecated. That way, you can decide when you want to make change, as it would be a breaking change (but one that most usres would accept and appreciate) I'd love to help start a spring-data-ravendb community project, which would really need this sort of change first. Having a driver that conforms more to standard jvm language practices would make it easier for users to consider adopting RavenDB,.

ml054 commented 1 year ago

Hi,

@cmitchell Thanks for you suggestion. I really appreciate it.

Please have a look at this issue: https://issues.hibernatingrhinos.com/issue/RavenDB-19938/Redesign-client-API-around-creation-of-database.

It is c# "version" of this one.

As a part of RavenDB culture we are trying to keep API between different languages as close as possible to each other. We live in times when we choose technology/language to solve the problem. Smooth learning curve helps in easy transition between languages.

What do you think about submitting PR to ravendb/ravendb repository with DocumentStore builder? Not sure, if you already had a chance to play with C#, but it might start of very interesting adventure.

cmitchell commented 1 year ago

I'm a little confused - are you asking me to make 2 PRs, one for the jvm client and one for the c# code? I've never touched c# so I would at least like to submit the first PR against the java client first so I could incorporate any feedback you might have. Then it would be easier for me to transfer code that I know you like to c# and then put up a PR for the c# code. I would understand if you decided not to merge the jvm PR until the c# work is also done so that you could keep the two code bases in line. That would also motivate me to do it in the c# code so that I could see it on the java side quicker :)

Also, I see the c# ticket was opened 3 weeks ago. Is someone already working it? Submitting the jvm PR first could serve as a reference to someone who already knows c# if someone is working or wants to work the c# ticket you linked to.

ml054 commented 1 year ago

We sync changes from c# client to other by reviewing git diff from latest sync points. With such approach we easily port all features and assure it is in sync. So there is no need for you to submit PR to Java. You can play with c# directly (and sync will be done after some period of time in batch along with other changes).

No one with working on that ticket right now, so if you don't mind feel free to submit your PR.

Personally I was Java developer and they I tried c#, so right now I know both and I don't regret. This is perfect opportunity to gather experience in your professional carrier. :)

ml054 commented 1 year ago

Oh, my bad. The pasted link is for creating new database, but you was referring to DocumentStore. Sorry for that.

As result feel free to create such builder in Java (and ignore c#) and let's leave original version to keep combability.

cmitchell commented 1 year ago

I've read the contribution guidlines and signed the CLA. The guidelines say I need to reference a ravendb ticket in my commit message(s) so should I use this issue, #66, or the one you linked to, [RavenDB-19938] )?

ml054 commented 1 year ago

@cmitchell Please use this issue as reference: https://issues.hibernatingrhinos.com/issue/RDBC-678/Add-a-DocumentStore.Builder-in-Java-client

ml054 commented 2 months ago

Closing due to inactivity.