Skip to content
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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ When updating the changelog, remember to be very clear about what behavior has c
and what APIs have changed, if applicable.

## [Unreleased]
- For entity level action request builders, issue a sensible error if the id is null.

## [29.22.10] - 2021-10-20
- SmoothRateLimiter - do not double-count execution delays on setRate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public void channelActive(ChannelHandlerContext ctx)

public static void processChannelActive(ChannelHandlerContext ctx, Logger log, ChannelPromise upgradePromise)
{
// For an upgrade request, clients should use an OPTIONS request for path “*” or a HEAD request for “/”.
// For an upgrade request, clients should use an OPTIONS request for path "*" or a HEAD request for "/".
// RFC: https://tools.ietf.org/html/rfc7540#section-3.2
// Implementation detail: https://http2.github.io/faq/#can-i-implement-http2-without-implementing-http11
final DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ public ActionRequest<V> build()

}

protected boolean hasId()
{
return _id != null;
}

private Map<FieldDef<?>, Object> buildReadOnlyActionParameters()
{
return buildReadOnlyActionParameters(_actionParams);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
Copyright (c) 2021 LinkedIn Corp.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package com.linkedin.restli.client.base;
haroldl marked this conversation as resolved.
Show resolved Hide resolved

import com.linkedin.restli.client.ActionRequest;
import com.linkedin.restli.client.RestliRequestOptions;
import com.linkedin.restli.common.ResourceSpec;

/**
* The abstract base class for all generated request builders classes for entity-level actions.
*
* For entity-level actions the action is being performed against a specific record which means
* that the key/id must be present in the request to identify which record to perform the action
* on. This class adds validation when the request is built that the id was set to a non-null
* value.
*/
public abstract class EntityActionRequestBuilderBase<K, V, RB extends ActionRequestBuilderBase<K, V, RB>>
extends ActionRequestBuilderBase<K, V, RB>
{
protected EntityActionRequestBuilderBase(String baseUriTemplate, Class<V> elementClass, ResourceSpec resourceSpec,
RestliRequestOptions requestOptions)
{
super(baseUriTemplate, elementClass, resourceSpec, requestOptions);
}

@Override
public ActionRequest<V> build() {
if (!hasId())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last request on this:

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
throw new IllegalStateException("Entity-level action request is missing required id value.");
}
return super.build();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exampleActionThatIsExplicitlyAnyLevel",
"doc" : "An example action on the greeting which is explicitly set to any-level.",
"parameters" : [ {
"name" : "param1",
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exampleActionThatIsExplicitlyEntityLevel",
"doc" : "An example action on the greeting which is explicitly set to entity-level.",
"parameters" : [ {
"name" : "param1",
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exceptionTest",
"doc" : "An example action throwing an exception."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,22 @@
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exampleActionThatIsExplicitlyAnyLevel",
"doc" : "An example action on the greeting which is explicitly set to any-level.",
"parameters" : [ {
"name" : "param1",
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exampleActionThatIsExplicitlyEntityLevel",
"doc" : "An example action on the greeting which is explicitly set to entity-level.",
"parameters" : [ {
"name" : "param1",
"type" : "int"
} ],
"returns" : "int"
}, {
"name" : "exceptionTest",
"doc" : "An example action throwing an exception."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.linkedin.restli.common.PatchRequest;
import com.linkedin.restli.examples.greetings.api.Greeting;
import com.linkedin.restli.examples.greetings.api.Tone;
import com.linkedin.restli.server.ResourceLevel;
import com.linkedin.restli.server.RestLiServiceException;
import com.linkedin.restli.server.UpdateResponse;
import com.linkedin.restli.server.annotations.Action;
Expand Down Expand Up @@ -96,6 +97,24 @@ public int exampleAction(@ActionParam("param1") int param1)
return param1 * 10;
}

/**
* An example action on the greeting which is explicitly set to entity-level.
*/
@Action(name="exampleActionThatIsExplicitlyEntityLevel", resourceLevel=ResourceLevel.ENTITY)
public int exampleActionThatIsExplicitlyEntityLevel(@ActionParam("param1") int param1)
{
return param1 * 11;
}

/**
* An example action on the greeting which is explicitly set to any-level.
*/
@Action(name="exampleActionThatIsExplicitlyAnyLevel", resourceLevel=ResourceLevel.ANY)
public int exampleActionThatIsExplicitlyAnyLevel(@ActionParam("param1") int param1)
{
return param1 * 12;
}

/**
* An example action throwing an exception.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ public void testActionRequestOptionsPropagation(RootBuilderWrapper<Long, Greetin
Assert.assertEquals(request.getRequestOptions(), builders.getRequestOptions());
}

@Test(dataProvider = com.linkedin.restli.internal.common.TestConstants.RESTLI_PROTOCOL_1_2_PREFIX + "requestBuilderDataProvider",
expectedExceptions = { IllegalStateException.class },
expectedExceptionsMessageRegExp = "Entity-level action request is missing required id value\\.")
public void testEntityActionRequiresIdInBuild(RootBuilderWrapper<Long, Greeting> builders)
{
builders.action("SomeAction").build();
}

@Test(dataProvider = com.linkedin.restli.internal.common.TestConstants.RESTLI_PROTOCOL_1_2_PREFIX + "requestBuilderDataProvider")
public void testGetRequestOptionsPropagation(RootBuilderWrapper<Long, Greeting> builders)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,24 @@ public void testRootSimpleResourceIntAction(RootBuilderWrapper<Void, Greeting> b
Assert.assertEquals(responseFuture.getResponse().getEntity().intValue(), 10);
}

@Test(dataProvider = com.linkedin.restli.internal.common.TestConstants.RESTLI_PROTOCOL_1_2_PREFIX + "requestBuilderDataProvider")
public void testRootSimpleResourceEntityAction(RootBuilderWrapper<Void, Greeting> builders) throws RemoteInvocationException
{
Request<Integer> request = builders.<Integer>action("ExampleActionThatIsExplicitlyEntityLevel").setActionParam("Param1", 3).build();
ResponseFuture<Integer> responseFuture = getClient().sendRequest(request);
Assert.assertEquals(responseFuture.getResponse().getStatus(), 200);
Assert.assertEquals(responseFuture.getResponse().getEntity().intValue(), 33);
}

@Test(dataProvider = com.linkedin.restli.internal.common.TestConstants.RESTLI_PROTOCOL_1_2_PREFIX + "requestBuilderDataProvider")
public void testRootSimpleResourceAnyAction(RootBuilderWrapper<Void, Greeting> builders) throws RemoteInvocationException
{
Request<Integer> request = builders.<Integer>action("ExampleActionThatIsExplicitlyAnyLevel").setActionParam("Param1", 3).build();
ResponseFuture<Integer> responseFuture = getClient().sendRequest(request);
Assert.assertEquals(responseFuture.getResponse().getStatus(), 200);
Assert.assertEquals(responseFuture.getResponse().getEntity().intValue(), 36);
}

@Test(dataProvider = com.linkedin.restli.internal.common.TestConstants.RESTLI_PROTOCOL_1_2_PREFIX + "requestBuilderDataProvider")
public void testRootSimpleResourceActionException(RootBuilderWrapper<Void, Greeting> builders) throws RemoteInvocationException
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import com.linkedin.pegasus.generator.JavaDataTemplateGenerator;
import com.linkedin.pegasus.generator.TemplateSpecGenerator;
import com.linkedin.pegasus.generator.spec.ClassTemplateSpec;
import com.linkedin.restli.client.ActionRequestBuilder;
import com.linkedin.restli.client.OptionsRequestBuilder;
import com.linkedin.restli.client.RestliRequestOptions;
import com.linkedin.restli.client.base.ActionRequestBuilderBase;
Expand All @@ -57,6 +58,7 @@
import com.linkedin.restli.client.base.CreateIdEntityRequestBuilderBase;
import com.linkedin.restli.client.base.CreateIdRequestBuilderBase;
import com.linkedin.restli.client.base.DeleteRequestBuilderBase;
import com.linkedin.restli.client.base.EntityActionRequestBuilderBase;
import com.linkedin.restli.client.base.FindRequestBuilderBase;
import com.linkedin.restli.client.base.GetAllRequestBuilderBase;
import com.linkedin.restli.client.base.GetRequestBuilderBase;
Expand Down Expand Up @@ -1378,22 +1380,27 @@ private void generateActions(JDefinedClass facadeClass,
{
for (ActionSchema action : resourceActions)
{
generateActionMethod(facadeClass, baseUriExpr, _voidClass, action, resourceSpecField, resourceName, pathKeys, pathKeyTypes, assocKeyTypes, pathToAssocKeys, requestOptionsExpr, rootPath);
generateActionMethod(facadeClass, baseUriExpr, _voidClass, ActionRequestBuilderBase.class, action,
resourceSpecField, resourceName, pathKeys, pathKeyTypes, assocKeyTypes, pathToAssocKeys, requestOptionsExpr,
rootPath);
}
}

if (entityActions != null)
{
for (ActionSchema action : entityActions)
{
generateActionMethod(facadeClass, baseUriExpr, keyClass, action, resourceSpecField, resourceName, pathKeys, pathKeyTypes, assocKeyTypes, pathToAssocKeys, requestOptionsExpr, rootPath);
generateActionMethod(facadeClass, baseUriExpr, keyClass, EntityActionRequestBuilderBase.class, action,
resourceSpecField, resourceName, pathKeys, pathKeyTypes, assocKeyTypes, pathToAssocKeys, requestOptionsExpr,
rootPath);
}
}
}

private void generateActionMethod(JDefinedClass facadeClass,
JExpression baseUriExpr,
JClass keyClass,
Class<?> builderParentClass,
ActionSchema action,
JVar resourceSpecField,
String resourceName,
Expand All @@ -1406,7 +1413,7 @@ private void generateActionMethod(JDefinedClass facadeClass,
throws JClassAlreadyExistsException
{
final JClass returnType = getActionReturnType(facadeClass, action.getReturns());
final JClass vanillaActionBuilderClass = getCodeModel().ref(ActionRequestBuilderBase.class).narrow(keyClass, returnType);
final JClass vanillaActionBuilderClass = getCodeModel().ref(builderParentClass).narrow(keyClass, returnType);
final String actionName = action.getName();

final String actionBuilderClassName = CodeUtil.capitalize(resourceName) + "Do" + CodeUtil.capitalize(actionName) + METHOD_BUILDER_SUFFIX.get(_version);
Expand Down