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

The federated merge function 'ApplyViewToElementsFunction' does not serialise #3025

Closed
tb06904 opened this issue Oct 3, 2023 · 2 comments · Fixed by #3028
Closed

The federated merge function 'ApplyViewToElementsFunction' does not serialise #3025

tb06904 opened this issue Oct 3, 2023 · 2 comments · Fixed by #3028
Assignees
Labels
bug Confirmed or suspected bug
Milestone

Comments

@tb06904
Copy link
Member

tb06904 commented Oct 3, 2023

Describe the bug
When running a GetAllElements or GetElements operation on a federated store using the REST API a server 500 error occurs. As these now use the ApplyViewToElementsFunction by default it appears to be the issue as a google guava type cannot be serialised.

To Reproduce
Run a GetAllElements operation on a federated graph via the REST API.

Expected behavior
The operation returns a federated list of elements

Stack trace and errors
Error output:

{
    "statusCode": 500,
    "status": "Internal Server Error",
    "simpleMessage": "uk.gov.gchq.gaffer.exception.SerialisationException: (was java.lang.UnsupportedOperationException) (through reference chain: uk.gov.gchq.gaffer.federatedstore.operation.FederatedOperation[\"mergeFunction\"]->uk.gov.gchq.gaffer.federatedstore.util.ApplyViewToElementsFunction[\"requiredContextValues\"]->com.google.common.collect.RegularImmutableSet[3])"
}

Additional context
Likely due to missing Jackson module to help serialise the guava types. Adding the Jackson datatype for guava should hopefully fix it.

May also need to add the tag to the relevant method:

@JsonDeserialize(as = ImmutableSet.class)
@tb06904 tb06904 added the bug Confirmed or suspected bug label Oct 3, 2023
@tb06904 tb06904 added this to the v2.1.0 milestone Oct 3, 2023
@GCHQDeveloper314
Copy link
Member

GCHQDeveloper314 commented Oct 4, 2023

I'd suggest investigating if we need to use this Guava type or if a regular Java 8 type would work instead. It if it did and that fixed this error then it would be a preferred solution.

Most of the uses of Guava in Gaffer are not needed and seem to be an artifact of pre-Java 8 design. Once Gaffer moves to supporting only Java 11, Guava could probably be removed entirely. As such, we should avoid introducing code reliant on Guava if this can easily be avoided.

@tb06904
Copy link
Member Author

tb06904 commented Oct 5, 2023

I'd suggest investigating if we need to use this Guava type or if a regular Java 8 type would work instead. It if it did and that fixed this error then it would be a preferred solution.

Most of the uses of Guava in Gaffer are not needed and seem to be an artifact of pre-Java 8 design. Once Gaffer moves to supporting only Java 11, Guava could probably be removed entirely. As such, we should avoid introducing code reliant on Guava if this can easily be avoided.

Good point if we were using Java 9+ then this could be fixed by using a Set.of() which would produce the required immutable set as is I think Java 8 can use Collections.unmodifiableSet() instead so might be a better fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed or suspected bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants