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

Fix validation for ActionResult return types #496

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

evanw555
Copy link
Contributor

Validate the type parameter of ActionResult, not ActionResult itself. Also improve the build error message for cases where an Action method's return type is a custom typeref but the @Action parameter doesn't specify a returnTyperef attribute.

Context:

For an Action method that has a custom typeref return type (e.g. CustomLong) yet doesn't indicate in the @Action parameter that it has a typeref class (e.g. CustomLongRef.class), the error will show up as the following:

  • If the return type is CustomLong: com.linkedin.restli.server.ResourceConfigException: @Action method 'doAction' on class 'com.example.impl.ExampleResource' has an invalid return type 'com.example.CustomLong'. Expected a DataTemplate or a primitive
  • If the return type is ActionResult<CustomLong>: an unhandled reflection exception.

After this change, the error message will be this for both CustomLong and ActionResult<CustomLong>: com.linkedin.restli.server.ResourceConfigException: @Action method 'doAction' on class 'com.example.impl.ExampleResource' has an invalid return type 'com.example.CustomLong'. Expected a DataTemplate or a primitive, or expected returnTyperef to be specified in the @Action annotation.

A couple notes:

  • I'm not sure if getLogicalReturnClass(method) was used instead of the returnClass parameter (which is more accurate because it also unwraps ActionResult) on purpose, but using returnClass would be accurate as it would make validation consistent between return types T and ActionResult<T>.
  • I just added more text to the existing exception, but would it make sense to check if the custom coercer exists and to display a more specific error message?
  • This is backward-incompatible because it removes ActionResult from the set of acceptable Action return types (it shouldn't be there if we're checking the logical return type).

@evanw555 evanw555 added the backward-incompatible Changes/removes an existing API, requires major version bump. PRs with this label should be bundled. label Dec 22, 2020
@evanw555 evanw555 added the v30 Should be bundled with the version 30.0.0 release label Oct 27, 2021
@evanw555 evanw555 force-pushed the fix/action-typeref-return-validation branch from 6a0911a to bbc7a73 Compare November 30, 2021 00:44
@nickibi nickibi changed the base branch from master to release/v30 December 2, 2021 22:49
@nickibi nickibi changed the base branch from release/v30 to master December 2, 2021 22:58
Validate the type parameter of ActionResult, not ActionResult itself.
Also improve the build error message for cases where an Action method's
return type is a custom typeref but the @action parameter doesn't
specify a returnTyperef attribute.
@evanw555 evanw555 force-pushed the fix/action-typeref-return-validation branch from bbc7a73 to ae39d60 Compare December 2, 2021 23:12
@nickibi nickibi changed the base branch from master to release/v30 December 2, 2021 23:13
@nickibi nickibi merged commit 982337f into release/v30 Dec 2, 2021
@nickibi nickibi deleted the fix/action-typeref-return-validation branch December 2, 2021 23:14
evanw555 added a commit that referenced this pull request Dec 3, 2021
Validate the type parameter of ActionResult, not ActionResult itself.
Also improve the build error message for cases where an Action method's
return type is a custom typeref but the @action parameter doesn't
specify a returnTyperef attribute.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompatible Changes/removes an existing API, requires major version bump. PRs with this label should be bundled. v30 Should be bundled with the version 30.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants