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

Gh-3253 Fix custom types in gremlin #3255

Merged
merged 7 commits into from
Jul 26, 2024

Conversation

tb06904
Copy link
Member

@tb06904 tb06904 commented Jul 19, 2024

Fixes the usage of custom Gaffer types not being able to be handled by a Gremlin client.

Works by simply making sure whenever conversion from Gaffer element to Tinkerpop element any custom types are converted to string representation and vice versa so that results can be parsed fine by a Gremlin client or Gaffer. This does mean slight overhead as conversions will happen even if Gremlin does not return the step result to the client but there is no way round this as each Gremlin step in theory can be returned so always needs any custom types converted.

Main type that is always converted is the Gaffer TypeSubTypeValue but more could be added, the string format has also been changed to be the same as the .toString so can more easily pass back in results, but a slight short hand is also accepted in Gremlin queries like [type=thetype,subType=thesubtype,value=thevalue]

Related issue

@tb06904 tb06904 linked an issue Jul 19, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 90.24390% with 8 lines in your changes missing coverage. Please review.

Project coverage is 67.27%. Comparing base (0d7f7a7) to head (60a5705).

Files Patch % Lines
...rocess/traversal/util/GafferCustomTypeFactory.java 87.09% 2 Missing and 2 partials ⚠️
...a/uk/gov/gchq/gaffer/tinkerpop/GafferPopGraph.java 90.90% 1 Missing ⚠️
...affer/tinkerpop/generator/GafferEdgeGenerator.java 87.50% 0 Missing and 1 partial ⚠️
...er/tinkerpop/generator/GafferPopEdgeGenerator.java 87.50% 0 Missing and 1 partial ⚠️
.../tinkerpop/generator/GafferPopVertexGenerator.java 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #3255      +/-   ##
=============================================
- Coverage      67.28%   67.27%   -0.01%     
+ Complexity      2572     2569       -3     
=============================================
  Files            926      926              
  Lines          29738    29771      +33     
  Branches        3290     3289       -1     
=============================================
+ Hits           20009    20029      +20     
- Misses          8282     8289       +7     
- Partials        1447     1453       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@p29876
Copy link
Member

p29876 commented Jul 23, 2024

All looks fine, just need to remember to update the docs with the new TSTV String format.

Am I right in thinking this means you can no longer get a TSTV object from a GafferPop graph, you'll only ever get the String in case you're querying from the server?
If so, assuming actual TSTV objects is something users would want, should we add a helper method to create a TSTV from the String representation to save them from parsing it themselves on the other end? e.g. a new constructor or a static TypeSubTypeValue.from(tstvString)

@p29876
Copy link
Member

p29876 commented Jul 23, 2024

All looks fine, just need to remember to update the docs with the new TSTV String format.

Am I right in thinking this means you can no longer get a TSTV object from a GafferPop graph, you'll only ever get the String in case you're querying from the server? If so, assuming actual TSTV objects is something users would want, should we add a helper method to create a TSTV from the String representation to save them from parsing it themselves on the other end? e.g. a new constructor or a static TypeSubTypeValue.from(tstvString)

Ignore me, you can do that with the new custom types factory
I was thinking we could have something more concrete that would return a TSTV or throw if invalid or something but it would just be a duplication of this method and it's probably fine as is

p29876
p29876 previously approved these changes Jul 23, 2024
@tb06904
Copy link
Member Author

tb06904 commented Jul 24, 2024

All looks fine, just need to remember to update the docs with the new TSTV String format.

Am I right in thinking this means you can no longer get a TSTV object from a GafferPop graph, you'll only ever get the String in case you're querying from the server? If so, assuming actual TSTV objects is something users would want, should we add a helper method to create a TSTV from the String representation to save them from parsing it themselves on the other end? e.g. a new constructor or a static TypeSubTypeValue.from(tstvString)

You are right basically, if the results are returned as Tinkerpop a Vertex or Edge it will have any custom types made safe as strings, you can always pass these back through a Gaffer element generator to make them into a Gaffer Entity or Edge though which will convert the types back or use the static util method.

cn337131
cn337131 previously approved these changes Jul 25, 2024
@tb06904 tb06904 dismissed stale reviews from cn337131 and p29876 via 70580e2 July 25, 2024 11:26
Copy link

sonarcloud bot commented Jul 25, 2024

@wb36499 wb36499 merged commit f4d3738 into develop Jul 26, 2024
26 of 27 checks passed
@wb36499 wb36499 deleted the gh-3253-fix-custom-types-in-gremlin branch July 26, 2024 09:42
@cn337131 cn337131 added bug Confirmed or suspected bug tinkerpop Specific to/touches the tinkerpop module labels Aug 1, 2024
@cn337131 cn337131 added this to the v2.3.0 milestone Aug 13, 2024
wb36499 added a commit that referenced this pull request Aug 13, 2024
* prepare for next development iteration

* prepare for next development iteration

* prepare for next development iteration

* Gh-3244 fix ci versioning (#3245)

* move to revision property

* update ci

* update ignore

* Gh-3085 Fix payloadOperation being exposed by getOperations (#3247)

* Gh-3085 Fix payloadOperation being exposed by getOperations

* headers

* add test

* Gh-3220: Standardise GafferPop Tests (#3243)

* update graph IT tests

* Rename GraphIT -> GraphTest

* create graph IT

* Create Modern Federated example graph

run graph tests on multiple graph types

* Update federated IT tests

move non-federates specific tests to graph tests

* tidying

* remove public + spotless

* sonarcloud

* sonarcloud

* Use allEdges/allEntities

* tidy

---------

Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>

* Gh-3237 Implement DeleteElements for Map Store (#3239)

* Gh-3237 Implement DeleteElements for Map Store

* remove unused param

* spotless

* codecov and sonarcloud

* improve codecov and sonarcloud

* reduce test duplication and add to federated store

* checkstyle

* remove unused properties and improve check

* address PR comments and improve tests

* fix indent

* checkstyle

* copyright

* bump version

* address comments

* add comment

---------

Co-authored-by: cn337131 <141730190+cn337131@users.noreply.github.com>
Co-authored-by: tb06904 <141412860+tb06904@users.noreply.github.com>

* Gh-3238: target delete accumulo (#3249)

* Gh-3237 Implement DeleteElements for Map Store

* remove unused param

* spotless

* wip

* finish

* checkstyle

* codecov and sonarcloud

* improve codecov and sonarcloud

* reduce test duplication and add to federated store

* checkstyle

* remove unused properties and improve check

* Gh-3238 Implement DeleteElements for Accumulo

* add accumulo test to federated store

* remove commented test

* address comments

* address comments

* checkstyle

* checkstyle

* headers

* code cov

* remove public

* remove AccumuloClient

* code cov

* checkystle

* add test

---------

Co-authored-by: cn337131 <141730190+cn337131@users.noreply.github.com>

* Gh-3246: Add an explain step and GafferPop to REST API (#3248)

* basic rest controller for gremlin

* basic websocket endpoint for gremlin inside the rest api

* improvements to the gremlin ws endpoint

* basic hooks for storing last chain

* Add working explain endpoint and tidy websocket

* Add open telementry reporting

* overhaul gremlin user auth and move cypher translator

* basic testing for gremlin controller

* move to utils class

* basic testing updates

* update the options parsing and testing

* test tweaks

* updates from testing

* tweaks

* spotless

* address comments

---------

Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>

* Gh-3250: GetElementsWithinSet bug fix (#3251)

* initial testing and changes

* tidy up, remove bug test and copyright headers

* fix commented out lines

* additional federated tests

* reduce duplicate tests

* checkstyle

* add inOutType

* copyright

* fix version variable

* address comments

---------

Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>

* Gh-3253 Fix custom types in gremlin (#3255)

* add conversions so gafferpop edges and vertexes are graphson compliant

* update testing

* update testing

* javadoc

* make sure cypher types can be used correctly

* typo

* Gh-3259: Improve release process (#3260)

* update release pipelines

* simplify

* release notes fix

* remove comment

* address comments

* Make gremlin config message clearer (#3264)

* Gh-3258: Sonarcloud Issues (#3261)

* initial improvements

* more test tidy up

* more fixes

* improve coverage

* copyright

* fix DeleteAllData test

* checkstyle

* address comments

* address comments

* checkstyle

* checkstyle

---------

Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>

* Gh-3254: Standardise Spring REST user authentication (#3263)

* remove unnecessary controller interfaces and add http header setting

* test tweaks

* Update rest-api/spring-rest/src/main/java/uk/gov/gchq/gaffer/rest/controller/JobController.java

Co-authored-by: cn337131 <141730190+cn337131@users.noreply.github.com>

---------

Co-authored-by: cn337131 <141730190+cn337131@users.noreply.github.com>
Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>

* prepare release gaffer2-2.3.0

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: tb06904 <141412860+tb06904@users.noreply.github.com>
Co-authored-by: p29876 <165825455+p29876@users.noreply.github.com>
Co-authored-by: wb36499 <166839644+wb36499@users.noreply.github.com>
Co-authored-by: cn337131 <141730190+cn337131@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed or suspected bug tinkerpop Specific to/touches the tinkerpop module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TSTV can't deserialise when using a Gremlin connection
4 participants