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

Java: Improve fluent API naming #763

Closed
ThorbenLindhauer opened this Issue Mar 21, 2018 · 14 comments

Comments

5 participants
@ThorbenLindhauer
Member

ThorbenLindhauer commented Mar 21, 2018

The fluent API has room for misunderstanding which method actually performs the request.

For example, completing a task looks like this currently:

TaskEvent event = ..;
TasksClient client = ..;

client
  .complete(event)
  .execute();

However, client.complete(event); already appears to users as it should do the job.

We should find a way where it is clear which method invocation acutally triggers the request.

@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Mar 21, 2018

Alternatives:

client
  .newCompletion(event)
  .payload(..)
  .execute();
client.complete(event);
client.complete(event, command -> command.payload(..));
@berndruecker

This comment has been minimized.

Member

berndruecker commented Mar 21, 2018

Probably it would even work without the "new"?

client
  .completion(event)
  .payload(..)
  .execute();

And of course also

client
  .completion(taskId)
  .payload(..)
  .execute();
@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Mar 21, 2018

Sounds good.

Let's discuss completion by ID in a separate issue as this is another beast with other implications.

@meyerdan

This comment has been minimized.

Member

meyerdan commented Mar 21, 2018

Just FYI: I once showed the API to Jakob and he was totally confused by the "execute" method. He thought that it would "execute the process" or something.

@berndruecker

This comment has been minimized.

Member

berndruecker commented Mar 22, 2018

Let's discuss completion by ID in a separate issue as this is another beast with other implications.

OK - created #765. This is an important one I think.

@berndruecker

This comment has been minimized.

Member

berndruecker commented Mar 22, 2018

It is also confusing where the topic has to be handed in:

   WorkflowInstanceEvent event = zeebe.workflows().create("default-topic")
      .bpmnProcessId("hello")
      .payload("{\"a\":\"5\"}")
      .execute();

I get the impression that I create the topic. It should be much more

   WorkflowInstanceEvent event = zeebe.topic("default-topic").workflows().creation()
      .bpmnProcessId("hello")
      .payload("{\"a\":\"5\"}")
      .do();

or the like. By moving the topic out it would also be easy to use the "default-topic" if none is specified :-) See also https://forum.zeebe.io/t/why-not-automatically-create-default-topic/122

@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Mar 22, 2018

I think this is also related to whether we will have APIs that allow you to do things by reference.

Here is why it is like it is today: With the current value-based APIs, the topic is already part of the value, so the current API prevents the user from writing something as follows:

TaskEvent task = ..; // contains information that task belongs to topic foo
zeebe.topic("bar").tasks().completion(task).execute();
@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Mar 22, 2018

The reasoning for the current design behind things like zeebe.workflows().create("default-topic") is as follows:

All API interactions have mandatory and optional parameters. Every API is structured as follows:

Client client = ..; // for example zeebe.workflows()
client.action(non-fluent-param1, non-fluent-param2, ...)
  .fluent-param1(..)
  .fluent-param2(..)
  ...
  .execute();

Here, the non-fluent parameters are supposed to be those that are required, and the fluent parameters are optional. That way, users run into less errors with not providing all parameters, etc.

@menski

This comment has been minimized.

Member

menski commented Mar 22, 2018

Here, the non-fluent parameters are supposed to be those that are required, and the fluent parameters are optional.

I noticed that we do not follow this on all APIs, i.e. when I create a task subscription I have to specify the task type, lock owner and lock time but they are all fluent parameters.

@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Mar 22, 2018

Yes, I broke this convention where there would be plenty of mandatory parameters. Consistency would probably be better.

@berndruecker

This comment has been minimized.

Member

berndruecker commented Mar 22, 2018

I agree for the moment to better do not make exceptions for mandatory parameters. It also makes it much harder to change later. The downside is of course that users don't see in advance what is mandatory and the compiler cannot check it. But I do not have a good idea to solve that as I think the fluent builder is what we want to aim at after the experiences with the Camunda 7 API.

@berndruecker

This comment has been minimized.

Member

berndruecker commented Mar 22, 2018

`

I think this is also related to whether we will have APIs that allow you to do things by reference. Here is why it is like it is today: With the current value-based APIs, the topic is already part of the value, so the current API prevents the user from writing something as follows:

Hmm - understood. But actually wouldn't that simply also be a runtime error in this case? While I definitely see the advantage of compiler checking for what is correct (again), if it makes the API less readable it might not be worth it?

Probably the topic also has to move one level down?

`

TaskEvent task = ..; // contains information that task belongs to topic foo
zeebe.tasks().completion(task).topic("bar").execute();

And you just skip the topic whenever you don't need it (if its in the value or if you are using the default-topic)?

@Zelldon

This comment has been minimized.

Member

Zelldon commented Mar 23, 2018

I like the idea for only have one verb as the final/end (in stream API called terminal operation).
Here are my proposal for the api:

// create new task
        taskClient.new("task-topic", "task-type")
              .payload(/*stuff*/)  /* OPTIONAL */
              .header(/*stuff*/)  /* OPTIONAL */
              .header(/*stuff*/)  /* OPTIONAL */
              .async() /* OPTIONAL */
              .create();

// complete existing
        taskClient.task(taskEvent)
              .payload(/*stuff*/)   /* OPTIONAL */
              .async()          /* OPTIONAL */
              .complete();

// fail existing
        taskClient.task(taskEvent)
              .async()          /* OPTIONAL */
              .fail();


// retry existing
        taskClient.task(taskEvent)
              .payload(/*stuff*/)   /* OPTIONAL */
              .async()          /* OPTIONAL */
              .retry(int count);
                    

The async switch can be used to send the request asynchronously. In that case create or complete will return a future instead of the direct result.

@menski menski added this to the Client API Design milestone Apr 5, 2018

@ThorbenLindhauer

This comment has been minimized.

Member

ThorbenLindhauer commented Apr 26, 2018

Closing this in favour of #793 and #820.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment