Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1483

PropertyMapStep returns Map<String,E> but puts non String keys in it!

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.2
    • Fix Version/s: 3.3.0
    • Component/s: process
    • Labels:
      None

      Description

      PropertyMapStep.map has return type Map<String,E>, but if includeTokens is true:

      if (element instanceof VertexProperty) {
                          map.put(T.id, element.id());
                          map.put(T.key, ((VertexProperty) element).key());
                          map.put(T.value, ((VertexProperty) element).value());
                      } else {
                          map.put(T.id, element.id());
                          map.put(T.label, element.label());
                      }
      

      T.id, T.key and T.value are NOT strings, so code looping through the keys in Java fails. toString() are missing... But do we rely on having these keys in other operations?

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user JPMoresmau opened a pull request:

          https://github.com/apache/tinkerpop/pull/446

          TINKERPOP-1483: valueMap should always return string keys

          Code changed, test added

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/JPMoresmau/tinkerpop TINKERPOP-1483

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/446.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #446


          commit 02a179ca3aba5089faca25f20647ff7dc1de1e6a
          Author: jpmoresmau <jp@moresmau.fr>
          Date: 2016-09-30T14:49:49Z

          valueMap should always return string keys


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user JPMoresmau opened a pull request: https://github.com/apache/tinkerpop/pull/446 TINKERPOP-1483 : valueMap should always return string keys Code changed, test added You can merge this pull request into a Git repository by running: $ git pull https://github.com/JPMoresmau/tinkerpop TINKERPOP-1483 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/446.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #446 commit 02a179ca3aba5089faca25f20647ff7dc1de1e6a Author: jpmoresmau <jp@moresmau.fr> Date: 2016-09-30T14:49:49Z valueMap should always return string keys
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Hm. This is a hard pill to swallow as its not backwards compatible. I was thinking you were going to go the route of making it `Map<Object,Object>` and thus, allow the enums as they are. If people have code that is `map.get(id)`, your changes will break their code.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 Hm. This is a hard pill to swallow as its not backwards compatible. I was thinking you were going to go the route of making it `Map<Object,Object>` and thus, allow the enums as they are. If people have code that is `map.get(id)`, your changes will break their code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JPMoresmau commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Well, I wasn't going to change the API on my very first PR . The first thing I did with a valueMap result was to iterate on the keys, which crashes... Of course if you want to change the API to Map<Object,Object> it's fine.

          Show
          githubbot ASF GitHub Bot added a comment - Github user JPMoresmau commented on the issue: https://github.com/apache/tinkerpop/pull/446 Well, I wasn't going to change the API on my very first PR . The first thing I did with a valueMap result was to iterate on the keys, which crashes... Of course if you want to change the API to Map<Object,Object> it's fine.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          I've seen lots of code where people actually use `id` as a property name.

          ```
          gremlin> g = TinkerGraph.open().traversal()
          ==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
          gremlin> g.addV().property("id", UUID.randomUUID())
          ==>v[0]
          gremlin> g.V().valueMap(true)
          ==>[id:0,id:[02f29cc6-7f04-477c-a884-d5196064109a],label:vertex]
          gremlin> g.V().project("vertexId","applicationId").by(id).by("id").next()
          ==>vertexId=0
          ==>applicationId=02f29cc6-7f04-477c-a884-d5196064109a
          ```

          Those people would be in big trouble if we would merge this PR:

          VOTE: -1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 I've seen lots of code where people actually use `id` as a property name. ``` gremlin> g = TinkerGraph.open().traversal() ==>graphtraversalsource[tinkergraph [vertices:0 edges:0] , standard] gremlin> g.addV().property("id", UUID.randomUUID()) ==>v [0] gremlin> g.V().valueMap(true) ==>[id:0,id: [02f29cc6-7f04-477c-a884-d5196064109a] ,label:vertex] gremlin> g.V().project("vertexId","applicationId").by(id).by("id").next() ==>vertexId=0 ==>applicationId=02f29cc6-7f04-477c-a884-d5196064109a ``` Those people would be in big trouble if we would merge this PR: VOTE: -1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Yea, this is a troublesome PR. VOTE -1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 Yea, this is a troublesome PR. VOTE -1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JPMoresmau commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Fine, but if you don't want to change the implementation to match the interface, you'll have to change the interface of valueMap... Having keys of a type that is not allowed by the actual generic signature of the map is not great...

          Show
          githubbot ASF GitHub Bot added a comment - Github user JPMoresmau commented on the issue: https://github.com/apache/tinkerpop/pull/446 Fine, but if you don't want to change the implementation to match the interface, you'll have to change the interface of valueMap... Having keys of a type that is not allowed by the actual generic signature of the map is not great...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          To support something like `valueMap(T.label, "name")`? This would be cool, but would go into another PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 To support something like `valueMap(T.label, "name")`? This would be cool, but would go into another PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JPMoresmau commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          What? I'm just saying that currently valueMap returns `Map<String,Object>`, so if you iterate on all the keys as String, you get a runtime crash because there are keys that are not strings. So if you don't want to put only String as keys, you need to change the Map to `Map<Object,Object>` , which will also break existing code.

          Show
          githubbot ASF GitHub Bot added a comment - Github user JPMoresmau commented on the issue: https://github.com/apache/tinkerpop/pull/446 What? I'm just saying that currently valueMap returns `Map<String,Object>`, so if you iterate on all the keys as String, you get a runtime crash because there are keys that are not strings. So if you don't want to put only String as keys, you need to change the Map to `Map<Object,Object>` , which will also break existing code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Oh, now I see what you're saying. Yes, this would be the right way to go. The current `PropertyMapStep` implementation is pretty weird.

          ```java
          protected Map<String, E> map(...)

          { final Map<Object, Object> map = new HashMap<>(); ... return (Map) map; }

          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 Oh, now I see what you're saying. Yes, this would be the right way to go. The current `PropertyMapStep` implementation is pretty weird. ```java protected Map<String, E> map(...) { final Map<Object, Object> map = new HashMap<>(); ... return (Map) map; } ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Can this PR be updated with Map<Object, Object> or should it be closed and open a new one?

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/446 Can this PR be updated with Map<Object, Object> or should it be closed and open a new one?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          This is interesting. I was unaware of some or didn't understand some implementation details. Taking a step back and looking at this again, I wonder if the original intent is more correct. Sorry @JPMoresmau
          :smiley:

          We have to ask what is the purpose of `Hidden`? And what is the purpose of `T..getAccessor()`? Looks like `Has` step internally use `T..getAccessor()` for equality tests. Is this more of an internal method or should it be exposed?

          Ultimately, the question is with what or how do we expect to access `T` tokens in a value map?

          Given: `map = g.V().valueMap(true).next()`

          Access by: `map.get(T.id)` *or* `map.get(T.id.getAccessor())`?

          If the answer is `T.id` then the interface must be <Object,Object>

          If the answer is `T.id.getAccessor()` then the interface must be <String,Object>

          In either case, there is no conflict between system-level `T` tokens (e.g. `T.id`), as these are either Enum or `Hidden`, and user-level strings (e.g. `id`).

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/446 This is interesting. I was unaware of some or didn't understand some implementation details. Taking a step back and looking at this again, I wonder if the original intent is more correct. Sorry @JPMoresmau :smiley: We have to ask what is the purpose of `Hidden`? And what is the purpose of `T. .getAccessor()`? Looks like `Has` step internally use `T. .getAccessor()` for equality tests. Is this more of an internal method or should it be exposed? Ultimately, the question is with what or how do we expect to access `T` tokens in a value map? Given: `map = g.V().valueMap(true).next()` Access by: `map.get(T.id)` * or * `map.get(T.id.getAccessor())`? If the answer is `T.id` then the interface must be <Object,Object> If the answer is `T.id.getAccessor()` then the interface must be <String,Object> In either case, there is no conflict between system-level `T` tokens (e.g. `T.id`), as these are either Enum or `Hidden`, and user-level strings (e.g. `id`).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          > Access by: `map.get(T.id)` *or* `map.get(T.id.getAccessor())`?

          By `map.get(T.id)`. Providers may allow property names that match TinkerPop's token accessors, hence `map.get(T.id)` must not be the same as `map.get("id")` / `map.get(T.id.getAccessor())` and a user-defined `id` property may not overwrite and may not be overwritten by the internal element id.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 > Access by: `map.get(T.id)` * or * `map.get(T.id.getAccessor())`? By `map.get(T.id)`. Providers may allow property names that match TinkerPop's token accessors, hence `map.get(T.id)` must not be the same as `map.get("id")` / `map.get(T.id.getAccessor())` and a user-defined `id` property may not overwrite and may not be overwritten by the internal element id.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Thanks for the clarification @dkuppitz

          `./docker/build.sh -t -i -n ` passes

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/446 Thanks for the clarification @dkuppitz `./docker/build.sh -t -i -n ` passes VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Yea, `Map<Object,Object>` is the thing. :| ... I really don't like `valueMap()`. For this reason and for the `boolean` argument overload ... fuggly.

          VOTE +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 Yea, `Map<Object,Object>` is the thing. :| ... I really don't like `valueMap()`. For this reason and for the `boolean` argument overload ... fuggly. VOTE +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          This requires CHANGELOG and upgrade docs too (unless a committer wants to take responsibility for that).

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/446 This requires CHANGELOG and upgrade docs too (unless a committer wants to take responsibility for that).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          @dkuppitz – will you handle merge and thus, do the CHANGELOG and update the upgrade docs on merge?

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 @dkuppitz – will you handle merge and thus, do the CHANGELOG and update the upgrade docs on merge?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user JPMoresmau commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          I've update the changelog and upgrade doc, not sure it's sufficient, please check! thanks!

          Show
          githubbot ASF GitHub Bot added a comment - Github user JPMoresmau commented on the issue: https://github.com/apache/tinkerpop/pull/446 I've update the changelog and upgrade doc, not sure it's sufficient, please check! thanks!
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/446#discussion_r89863961

          — Diff: CHANGELOG.asciidoc —
          @@ -46,6 +46,7 @@ TinkerPop 3.3.0 (Release Date: NOT OFFICIALLY RELEASED YET)

          • Removed `tryRandomCommit()` from `AbstractGremlinTest`.
          • Changed `gremlin-benchmark` system property for the report location to `benchmarkReportDir` for consistency.
          • Added SysV and systemd init scripts.
            +* `valueMap` now returns a `Map<Object,E>` since keys could be `T.id` or `T.label`.
              • End diff –

          Please make it read `GraphTraversal.valueMap()`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/446#discussion_r89863961 — Diff: CHANGELOG.asciidoc — @@ -46,6 +46,7 @@ TinkerPop 3.3.0 (Release Date: NOT OFFICIALLY RELEASED YET) Removed `tryRandomCommit()` from `AbstractGremlinTest`. Changed `gremlin-benchmark` system property for the report location to `benchmarkReportDir` for consistency. Added SysV and systemd init scripts. +* `valueMap` now returns a `Map<Object,E>` since keys could be `T.id` or `T.label`. End diff – Please make it read `GraphTraversal.valueMap()`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          I just realized that `valueMap()` is `Map<String,Object>`, but `valueMap(boolean)` is `Map<Object,Object>`. We should maintain that level of explicitness as I suspect in the future `valueMap(boolean)` will change given that its sort of a wart right now.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 I just realized that `valueMap()` is `Map<String,Object>`, but `valueMap(boolean)` is `Map<Object,Object>`. We should maintain that level of explicitness as I suspect in the future `valueMap(boolean)` will change given that its sort of a wart right now.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/446#discussion_r89875173

          — Diff: gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyValueMapTest.groovy —
          @@ -42,5 +42,10 @@ public abstract class GroovyValueMapTest {
          public Traversal<Vertex, Map<String, List<String>>> get_g_VX1X_outXcreatedX_valueMap(final Object v1Id)

          { new ScriptTraversal<>(g, "gremlin-groovy", "g.V(v1Id).out('created').valueMap", "v1Id", v1Id) }

          +
          + @Override
          + public Traversal<Vertex, Map<Object, Object>> get_g_V_valueMapToken() {
          — End diff –

          This method is named wrong, it should be:

          ```
          get_g_V_hasLabelXpersonX_filterXoutEXcreatedXX_valueMapXtrueX()
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/446#discussion_r89875173 — Diff: gremlin-groovy-test/src/main/groovy/org/apache/tinkerpop/gremlin/process/traversal/step/map/GroovyValueMapTest.groovy — @@ -42,5 +42,10 @@ public abstract class GroovyValueMapTest { public Traversal<Vertex, Map<String, List<String>>> get_g_VX1X_outXcreatedX_valueMap(final Object v1Id) { new ScriptTraversal<>(g, "gremlin-groovy", "g.V(v1Id).out('created').valueMap", "v1Id", v1Id) } + + @Override + public Traversal<Vertex, Map<Object, Object>> get_g_V_valueMapToken() { — End diff – This method is named wrong, it should be: ``` get_g_V_hasLabelXpersonX_filterXoutEXcreatedXX_valueMapXtrueX() ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/446#discussion_r89875249

          — Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java —
          @@ -125,6 +131,30 @@ public void g_VX1X_outXcreatedX_valueMap() {

          }

          + /**
          + * TINKERPOP-1483
          + *
          + */
          + @Test
          + @LoadGraphWith(MODERN)
          + public void valueMapHasObjectKeys() {
          — End diff –

          This method is named wrong, it should be like the traversal generator method name, minus the `get_` prefix.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/446#discussion_r89875249 — Diff: gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/ValueMapTest.java — @@ -125,6 +131,30 @@ public void g_VX1X_outXcreatedX_valueMap() { } + /** + * TINKERPOP-1483 + * + */ + @Test + @LoadGraphWith(MODERN) + public void valueMapHasObjectKeys() { — End diff – This method is named wrong, it should be like the traversal generator method name, minus the `get_` prefix.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          This is looking really good. We need one more VOTE.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/446 This is looking really good. We need one more VOTE.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Hold on, I've been busy with other stuff. I will look into this PR today.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 Hold on, I've been busy with other stuff. I will look into this PR today.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/446

          Looked through the code, did some manual test - all good.

          VOTE: +1

          Gonna merge it in a few.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/446 Looked through the code, did some manual test - all good. VOTE: +1 Gonna merge it in a few.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/446

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/446

            People

            • Assignee:
              dkuppitz Daniel Kuppitz
              Reporter:
              JPMoresmau JP Moresmau
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development