search

ArangoDB-Community / arangodb-tinkerpop-provider

An implementation of the Tinkerpop OLTP Provider for ArangoDB
Apache License 2.0
84 stars 16 forks source link

new Config Option to determine whether colelctions should be prefixed or not #42

Closed fdominik closed 5 years ago

fdominik commented 5 years ago

as discussed in #40 . However proper testing is still needed.

arcanefoam commented 5 years ago

Hi, Thanks for the patch. I'll review and run the Tinkerpop tests with the new option and see how it goes.

fdominik commented 5 years ago

So I found that it doesn't work as I expected, let me push some more fixes today and I will let you know when it is ready... The commits should be automatically appended to this Pull Request...

fdominik commented 5 years ago

ok it should be fixed now... So please run the Tinkerpop tests... And if you have any link or your own guide on how to run these tests, what to download etc..., would you put it to the wiki? So that others have it also?

arcanefoam commented 5 years ago

Hi, Any chance you have an obfuscated small example DB to try this?

fdominik commented 5 years ago

Sure, I downloaded the JSONs from Arango and added the Test JAVA code I am using... https://github.com/fdominik/arangodb-tinkerpop-provider/raw/fdominik-example-branch/pr-42_new-config-example.zip

arcanefoam commented 5 years ago

Hi, I have ran the ClientTest tests with your patch (I have made them parametric to test the flag possibilities) and everything works ok. However, I am wondering if what we really want is "useExistingGraph" rather than "shouldPrefixCollectionNames" (seems to me this more closely describes what the use case is) Further, with "shouldPrefixCollectionNames" the ArangoDBGraph initialisation will fail if you don't create the properties and variables collections. With the "useExistingGraph", it would be more clear that I want to reuse the existing collections and hence this additional required collections could be created for me (instead of exploding in my face).

fdominik commented 5 years ago

Well useExistingGraph means that there already exists a graph and you want to reuse it. This is not true in my opinion, you just want to reuse existing collections, but there still will be a new graph created. So I would maybe prefer "useExistingCollections" or maybe ""shareCollectionsAmongGraphs"?

Regarding Required collections, I am not sure I understand what you mean. I deleted _graph_ELEMENT-HAS-PROPERTIES, graph_ELEMENT-PROPERTIES, graphGRAPH-VARIABLES collections, left only _'person', 'document', 'foundat' collections. And it worked. However when I deleted also the standard collections (e.g. 'person'), it fails. But it fails even if I set .shouldPrefixCollectionNamesWithGraphName(true)... So it seems this is not related to this fix, it has been there before? It is in ArangoDBUtil here:

if (!graph.getVertexCollections().containsAll(allVertexCollections)) {
            Set<String> avc = new HashSet<>(allVertexCollections);
            avc.removeAll(graph.getVertexCollections());
            throw new ArangoDBGraphException("Not all declared vertex names appear in the graph. Missing " + avc);
        }
arcanefoam commented 5 years ago

Hi,OK I agree we only want to reuse/share existing vertex/edge collections. With that in mind:

In ArangoDBGraph aprox line 580 in the constructor I have this check: if (graph.exists()) { this.name = graph.name(); ArangoDBUtil.checkGraphForErrors(vertexCollections, edgeCollections, relations, graph, options, shouldPrefixCollectionNames); ArangoCursor<String> iter = client.getGraphVariablesId(this.name); if (iter.hasNext()) { this.variables_id = iter.next(); } else { throw new ArangoDBGraphException("Existing graph does not have a Variables collection"); } } and the checkGraphForErrors method verifies that the required collections exist. However, at the moment this only is checked if a graph with the configured name is found in the database.

Since the proposed flag allows us to reuse/share existing collections, I guess a similar check for these collections would have to happen.

fdominik commented 5 years ago

So I've just comited a fix, which I found when testing #38, and now I did a proper testing of following scenarios, with the expected outcome:

It behaves the same way as if it would prefix the collections. That means, it fails only if the graph exists but some collections are missing. Which is correct. So if you are getting some errors, just delete the graph.

This brings me to the point, that probably a Test of Property on Property is missing, because you said all Unit Tests pass, but they wouldn't if property on property would be tested on my PR.

So now I don't think the check you mentioned in ArangoDBGraph needs any modification, because it works as it is.

What I am still not sure is the final naming of the new Config Option.

I ticked the one I prefer (and additional JavaDoc + Wiki page would be created so that people better understand the behavior).

fdominik commented 5 years ago

Hi @arcanefoam I see that you merged some parts of this PR... However I still see that 3 commits aren't part of it. I don't know why, in github UI it seems you did Merge after I pushed them, however in GIT client I see that you merged with beece57 commit. So maybe cherry-pick these 3 commits would be usefull? 1eb0895 f9fe26a 05fc8c2

Or merging the fdominik/develop branch from my fork should do the trick as well.

arcanefoam commented 5 years ago

Hi, yes I am missing those commits because I am working in the vertex properties as well and some big changes in the client that changed some of the ways the prefix flag is used.

I haven't forgot about you, but as mentioned somewhere these changes are trickier than foreseen ;)

arcanefoam commented 5 years ago

As a closing note, the proposed idea has been merged into version 2.0.2, although some major implementation changes were done .