v 4: Fluent-style API (early preview). feedback welcome!

[alexeyOnGitHub]

fluent-style API with "active" domain objects (Issue, User, Membership, etc) which have methods for CRUD operations embedded in them. this replaces xxManager classes for CRUD operations.

note - not ALL methods are implemented yet. this is more of a demo of the proposed approach.

a few examples of old vs. new API:

old:

    Issue issue = IssueFactory.createWithSubject("test123");
    Version ver = VersionFactory.create(123);
    issue.setTargetVersion(ver);
    IssueCategory cat = IssueCategoryFactory.create(673);
    issue.setCategory(cat);
    issue.setProject(project);
    manager.getIssueManager().createIssue(issue);

new API:

   Issue issue = new Issue(mgr.getTransport(), projectId)
        .setSubject("test123")
    .setTargetVersion(new Version().setId(123))
    .setCategory(cat);
        .setProjectId(projectId)
        .create();

old API:

    Group template = GroupFactory.create("group " + System.currentTimeMillis());
    Group group = userManager.createGroup(template);

new API:

    Group group = new Group(transport)
        .setName("group")
        .create();

old API:

    userManager.deleteUser(123);

new:

    user.delete();

tagging a few people who recently submitted PRs to get early feedback: @taomWTP @marob @likeanowl @christau @albfan @matthiasblaesing

[svitkovsergey]

@alexeyOnGitHub seems pretty good! :+1:

[panekzogen]

Fluent style api looks much better and convenient than old.

[alexeyOnGitHub]

released version 4.0.0.preview.1 to OSS Sonatype. It will be auto-synced to Maven Central soon. feel free to try it out!

[matthiasblaesing]

I'm not sure whether I like it. My first gut feeling is not good.

One pattern that breaks with this fluent approach is:

1. Create DTO (for example a TimeEntry) on the Swing EDT, so that data from input fields can be directly transfered to the DTO
2. Dispatch the transfer of the DTO into a background thread.

With the fluent approach, I'll need three steps:

1. Aquire a redmine transport off the EDT in a background thread and fill the DTO
2. On the EDT transfer data from input field to the DTO
3. Dispatch the network interfacing thread again into a background thread

The alternative would be:

1. Create DTO with a `null' transport
2. Fill DTO with the data on the EDT
3. Dispatch the transfer into the background thread, there set the transport and invoke the target method

I see a benefit of the fluent approach, but I see problems, if the above two phases need to be separated.

Edit: I just noticed, that both approaches depicted above won't work. For example adding a watcher to an issue makes a network call. I basicly would have to put every interaction with the java redmine bindings into a background thread. I don't think, that this will work for me.

[alexeyOnGitHub]

@matthiasblaesing I see your point. the new API may require separating your internal domain objects structure from the library usage. this "fluent-style" makes the objects less reusable, but improves discoverability for API operations. it follows "OOP" approach it its strict sense (data and operations are combined together), instead of following currently widely used "anemic domain design" with POJOs and "manager" classes to operate on them. the obvious downside of this "fluent design" (which realistically we can just call "pure OOP" in this context) is that "active" objects lose a lot of their reusability in various contexts. is better discoverability a good enough benefit to make up for that?..

as much as I like anemic model, my experience of working with somewhat similar Atlassian's JIRA API makes me very averse to this approach for REST API usage due to a large amount of boilerplate and lots of "factories" and "managers".

if you are working on a swing-based app, I assume you perform all network-related operations (like creating issue, creating field, etc) in a separate thread and then later update UI when some "Future" completes.

if you need to add a watcher to an issue, that would mean a network call no matter what. the new API will require calling ".addWatcher()" or something like that, while the old one requires IssueManager.addWatcher(..). either way you need to keep some object with transport in it, and only use it in a separate thread to make sure your UI does not freeze.

if we were to move the API to the new model, would you have any suggestions on how to make it better for your use-case?

[matthiasblaesing]

@alexeyOnGitHub the benefit of the old API was, that there was a clear distinction between the passive objects, that were save to operate on in the EDT and the network interaction (the manager calls). With that distinction, I can work and ensure, that network calls are always done outside the EDT.

In an ideal world, redmine would just publish the API as a swagger document and I could generate my data + accessor objects from that. Having worked with WSDLs, generating code from these was always pretty easy.

I have no strong feelings here. My work on the redmine client was always motivated by curiosity, at this time I think I haven't looked at the codebase for at least a year and don't use it myself.

[javiertuya]

I have been using this api from several months ant it is great. And the fluent style looks better. Looking for simplicity and code reuse, why not move more to OO using inheritance, and maybe mitigate some of the @matthiasblaesing concerns? This is a proposal:

Convert the FluentStyle interface into an abstract class to encapsulate all common functionality related to transport. This base class would include:

• new() and new(transport). Do not specify any transport in subclass constructors. The transport variable will be protected in this base class.
• initCollections() and getId() abstract methods, that should be implemented by the beans that are who know the details of storage.
• Public methods create(), update(), delete(), plus versions with the transport parameter that perform the main operations. These methods will check if transport and the id have been set, raising an exception otherwise.
• Many methods in *Manager classes have been moved to the beans, but not the GET operations. A set of methods like getById() and getList(), maybe other versions with more parameters, will perform the basic functionality that is currenty in Manager classes.
• More complex operations should be implemented in the beans when necessary.

Therefore, the api could be called using alternative ways, eg:

new User(transport).setXXX().setYYY.update()
new User().setXXX().setYYY().update(transport)

The amount of bean’s code would be reduced and transport related operations abstracted as most as possible.

[alexeyOnGitHub]

using inheritance would remove a few lines, at the expense of making the code less flexible and much harder to change later.

overall, inheritance looks like a promising concept at first, but it is a misleading pattern which I prefer to avoid as much as I can. composition over inheritance is all the rage these days, I would say. inheritance looks great in "Animal - Cat - etc" samples, but over time it grows into a cludgy monster with multiple methods like "not implemented" , "not relevant - ignore" , etc - because world is rarely strictly hierarchical, relationships between objects may change, and code needs to adapt sooner or later and inheritance model makes it much more expensive than composition approach.