[
https://issues.apache.org/jira/browse/TINKERPOP3-695?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14563578#comment-14563578
]
stephen mallette commented on TINKERPOP3-695:
---------------------------------------------
this turned into way more than i wanted it to be and touched more code than i
wanted in doing it. but it had to be done as i discovered bugs and incorrect
semantics while trying to sort it out. sooo - heeeeerrrreeee we go......
I implemented this the way you had it at first and it worked, but something
didn't sit right with me about it. I think I'm having you twist the semantics
of {{supportsNumericIds}} by suggesting what i did above. I think that we've
largely thought of those particular features as being related to native storage
of the id not the types of identifiers that can be consumed (the javadoc for
those features reminded me of such as well as considering the code). As it
stands we force, some {{toString()}} equality of ids in the test suite which i
think is good, but that doesn't mean that a graph can claim it
{{supportsStringIds}}. I'm assuming that this is why you have to OptOut of
some of the tests in {{FeatureSupportTest}}.
Anyway, I think i have a different solution. From your side, please revert to
returning {{false}} for {{supportsNumericIds}} (since you store it internally
as String). If you like that you do the internal conversion and the tests
manage to pass, then you can keep that functionality (otherwise, perhaps you
should just revert it completely). Now, note that in {{IoTest}} the id
assertion function looks like this:
https://github.com/apache/incubator-tinkerpop/blob/32924d86ae98b112af9a0324e66d12930c2beeea/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/io/IoTest.java#L2357-L2371
Two big changes here:
1. fixed a bug where i was only checking on the edge feature for user supplied
ids
2. check that the graph supports numerics for ids if they do support user
supplied ids
This allows you to execute the {{IoTest}} and get the coverage without having
to worry about ids assertion. It's a gap, but our suite can only stretch so
far. I'd assume you would want to implement your own tests over IO to validate
ids if you so desired.
Now that brings us back to the original problem with returning {{false}} for
{{supportsNumericIds}}. If you do that then your graph won't load the data via
{{LoadGraphWith}} annotation and a good portion of the test suite won't
execute. To deal with this, I've modified {{Features}} for vertices, edges and
vertex properties. They each have a {{willAllowId(id)}} method which will test
an id to determine if the {{Graph}} will accept it via usage with {{T.id}}. I
then modified {{Attachable}} to only use the id assignment if that method
returns {{true}}. Graphs that return {{false}} for {{userSuppliedIds} will
immediately return {{false}} for this method so it should not break them.
This approach actually makes a bit more sense overall because we have this
notion of internal representation of an id versus what the graph will accept as
id. Those are technically two different things and the test suite needs to be
aware of that. The downside was that making these changes forced more
"feature" checks because rather than simply testing naively for
{{supportsUserSuppliedIds}} once and re-using that result, we have to
independently test each id as the type could change. Probably a worthy change
for whatever the cost is. If users want to forgo such checks for improved
speed they can supply their own methods for attachment (i.e. i know my source
data and i know my target graph so no need to check features).
Please let me know if this change helps resolve your problems.
> test not calling convertId()
> ----------------------------
>
> Key: TINKERPOP3-695
> URL: https://issues.apache.org/jira/browse/TINKERPOP3-695
> Project: TinkerPop 3
> Issue Type: Improvement
> Components: test-suite
> Reporter: Ran Magen
> Assignee: stephen mallette
> Fix For: 3.0.0.GA
>
>
> any test that uses Attachable to load its graph (e.g. using the Gryo loader)
> will not pass the id through the graph providers convertId(id). there are
> around 60 different tests affected.
> The offending line:
> https://github.com/apache/incubator-tinkerpop/blob/4bbbb6365f36c20ad63ca4665657fd1169a7d246/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/Attachable.java#L276
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
