Throw an exception when an entity-level action would be invoked witho…

[haroldl]

…ut an id.

Entity-level actions are performed on a specific resource, and so the id value
in the request is mandatory. Today, if the client forgets to call .id(...) to
supply a value the request can be built and sent to the server where it will
cause an error with a mysterious error message that implies that there is no
action of that name, i.e.
"POST operation named myAction not supported on resource '...' URI: '...'"

Here we fix that so that
a) The problem is caught on the client side during the RequestBuilder.build()
method call so that the request cannot be sent to the server.
b) A sensible error message is given where the stack trace's line number will
lead the developer straight to the root of the problem:
"Missing or null id value; we cannot invoke this entity-level action."

Summary

When invoking an entity-level action, if your code passes null to the request builder's id setter, then the request will fail with an error saying that the action does not exist. I propose a performant way to provide a better error message saying that because it is an entity-level action (as opposed to a collection-level action) a value for the key is required.

Problem statement

Suppose that we have "myResource" generated from net.hotelling.harold.restli.MyResource with entity-level action "myAction". For an entity level action, the following request will always fail:

    restClient.sendRequest(new MyResourceRequestBuilders().actionMyAction()
      .id(null)
      .build());

This is an easier bug to introduce if the id is supplied by a nullable variable where there may be some unintended code path that

    // Bad code path results in key == null here:
    restClient.sendRequest(new MyResourceRequestBuilders().actionMyAction()
      .id(key)
      .build());

Unfortunately, the error message that occurs in this scenario implies that the action does not exist, which should never happen because the action was invoked using client code generated from the server-side resource class.

com.linkedin.restli.server.RoutingException: POST operation named myAction not supported on resource 'net.hotelling.harold.resources.restli.MyResource' URI: 'http://localhost/myResource?action=myAction'

The error message is coming from this logic on the server side:

rest.li/restli-server/src/main/java/com/linkedin/restli/internal/server/RestLiRouter.java

Line 254 in b1ac6f1

throw new RoutingException(

Proposed solution

For entity-level actions, add a null check for the _id during the #build() that takes the RequestBuilder and creates an ActionRequest. If the _id is null, then throw a meaningful error message. Add the check by having the generated RequestBuilder class that models an entity level action subclass a new layer in the inheritance hierarchy. The new class will be EntityActionRequestBuilderBase which exists only to perform this check. Collection level actions will continue to subclass ActionRequestBuilderBase.

How is the proposed solution performant?

It will also perform the check in the Rest.li client side to save on unnecessary network traffic.
The additional logic will only exist for RequestBuilders classes for entity-level actions.
The additional null check is trivial in cost compared to other per-request logic already being performed in com.linkedin.restli.client.AbstractRequestBuilder#getReadOnlyOrCopyKeyObject, for example the two instanceof checks.

In our design do we need to consider a case where the same action name exists for both a collection and entity level action?

Code gen logic builds a class name that would be the same for an entity-level and collection-level action with the same action name, so we know that this cannot happen.
Doesn't matter anyway - we generate separate action request builder classes.

Are there alternative approaches?

After looking for a matching method if there is no match, loop again to see if there is a name-only match.
Or, since we know that there can only be one action with a given name:

    // https://github.com/linkedin/rest.li/blob/b1ac6f103e0248450f35bc618401166dcd38c830/restli-server/src/main/java/com/linkedin/restli/internal/server/model/ResourceModel.java#L428
    for (ResourceMethodDescriptor methodDescriptor : _resourceMethodDescriptors)
    {
      if (methodDescriptor.getType().equals(ResourceMethod.ACTION)
              && actionName.equals(methodDescriptor.getActionName()))
      {
        if (methodDescriptor.getActionResourceLevel().equals(resourceLevel)) {
          return methodDescriptor;
        } else {
          // TODO: return some meaningful error saying the action exists but the ResourceLevel does not match...
        }
      }
    }

[evanw555]

Great contribution. I have some nits plus three general comments:

1. Please update the CHANGELOG.md file with a user-friendly description of what this change is doing. Follow the instructions in the file, leave the bullet points under the Unreleased section.
2. This change is backward-incompatible because it (1) adds a new validation, and (2) alters the class hierarchy of generated request builders (class hierarchy alteration can cause problems in consumers). Thus, we can bundle this change with the next major release. We can review this, but wait until v30 to merge it.
3. It would be really great to also update the error message on the server side if possible. I agree that it's very uninformative and misleading... would you up for opening a new PR to solve this?

[haroldl]

Thanks for the review. I've updated the PR with changes to address your comments. Yes, I can pursue a separate PR to improve the error handling on the server side.

[junchuanwang]

Great contribution +1 ! Really liked your PR description

[junchuanwang]

what about entity level resource identified in the simple resource. It is a legit case but with no entity key. in this case resourcelevel.entity == resourcelevel.any;

Will this request builder also fail in this case?

[haroldl]

Good call, let me look into how the simple resource case works.

[haroldl]

I've just added a commit to this PR that shows that entity level actions in simple resources are still working with my change. I added integration tests for actions with both of the supported resource levels ANY and ENTITY.

The reason why this isn't broken by my other changes is kind of interesting. For simple resources, entity level actions in the restspec.json do not appear under the "entity" JSON key. They appear as top level actions similar to where collection level actions appear in a restspec.json for a collection resource. So from a code gen perspective they're not really entity level actions.

[junchuanwang]

One last request on this:

Can you add what you explain as a comment/documentation to the code?

[haroldl]

I've added to the class-level javadoc and pushed that commit. Take a look and let me know what you think.

[junchuanwang]

@evanw555 Is this technically backward compatible? Because incompatible cases will raise error in current version.

[haroldl]

@evanw555 Is this technically backward compatible? Because incompatible cases will raise error in current version.

Often times in Rest.li compatibility the focus is on client-server communications but that is not the case here. The problem here is when the API RequestBuilder classes are generated by a server using a newer version of Rest.li then they will inherit from the new EntityActionRequestBuilderBase class. If this generated code is used by a client that has a dependency on an older version of Rest.li, then their code will fail with a ClassNotFoundException when it tries to load that new class.

[junchuanwang]

@evanw555 Is this technically backward compatible? Because incompatible cases will raise error in current version.

Often times in Rest.li compatibility the focus is on client-server communications but that is not the case here. The problem here is when the API RequestBuilder classes are generated by a server using a newer version of Rest.li then they will inherit from the new EntityActionRequestBuilderBase class. If this generated code is used by a client that has a dependency on an older version of Rest.li, then their code will fail with a ClassNotFoundException when it tries to load that new class.

Thanks for the explanation. You are right, this is not an incompatibility about API, but still backward incompatible.

I am still wondering if we should do this as minor version upgrade since this incompatibility can be caught during build time and is not changing API behavior. WDYT @evanw555

[haroldl]

Another issue is that existing code with logic to handle this problem might have error handling tied to the response from the server and that logic would now be skipped due to an exception happening earlier, before the request is sent.

[evanw555]

@evanw555 Is this technically backward compatible? Because incompatible cases will raise error in current version.

Often times in Rest.li compatibility the focus is on client-server communications but that is not the case here. The problem here is when the API RequestBuilder classes are generated by a server using a newer version of Rest.li then they will inherit from the new EntityActionRequestBuilderBase class. If this generated code is used by a client that has a dependency on an older version of Rest.li, then their code will fail with a ClassNotFoundException when it tries to load that new class.

Thanks for the explanation. You are right, this is not an incompatibility about API, but still backward incompatible.

I am still wondering if we should do this as minor version upgrade since this incompatibility can be caught during build time and is not changing API behavior. WDYT @evanw555

@junchuanwang Theoretically we could, but I'd rather play it safe and bundle it with the major release. Code generation changes have caused a lot of pain in the past since the version alignment isn't totally guaranteed between build time and run time.

[junchuanwang]

Please fix the build failure/test in github actions

Also Evan @evanw555 Please work with customer for Linkedin wc-tests.

And then we should take it from there to arrange this commit to next major version release

[haroldl]

Please fix the build failure/test in github actions

Those two failing tests are passing for me locally. Could we retry the CI check?