Uh, I think we forgot to talk about user rights to access this.

You can see how it would be done here:
https://github.com/arangodb/arangodb/blob/b14b418811b3f4e292429310a5de41f78325b957/arangod/RestHandler/RestAdminClusterHandler.cpp#L2367C1-L2372C4

My suggestion right now:
RO on "canUseDatabase" and "RW" on canUseCollection" for the collection schema.
And RW on "canUseDatabase" for everything else.

We may lift this later.

I added permission check if blocks and will show them on Friday.

I would make this also return a Result, and move the complete "write response or write error" into the execute method.

I changed the logic of those helper methods and will show them on Friday.

This code is correct when looking into above method.
However this looks strange, as suggested above I would not have any function return the RestStatus.
But all functions return a Result.
And then in the execute react on the Result.fail() in one central place one time.
Then it is clear that on any error case the error message is written.

WDYT?

I agree that one central place should react all the Result.fail() so it will be clear that where the error came from. On the other hand, I called generateError() in the stemmed method so the error message is concrete.

🤔 I think having a view without links is valid.
it is not useful in production though, but we may just look at a bad point in time (e.g. during creation of the view)
I think I would let this through without an error.
Then of course if asking for the view it should give an empty list of collections.

I agree. So, I changed the method so when data doesn't have links yet, it will be an empty JSON array instead of generateError().

I think it is valid to have a View with "includeAllFields: true" would this error out here?

I hadn't been aware of includeAllFields actually. I implemented a method to fetch all the attributes and its analyzers with the limit of sampleNum, but I have some questions on this.

This code is correct.

However it would need to copy the information from the fieldsBuilder into the colBuilder, and then yet again into the linksBuilder.

Can you instead write the information directly into the links builder?
I think if you start with linksBuilder.openObject(); at the beginning of the loop you can write into it directly.

Maybe you want to even check the ObjectGuard (this directly does a close() for you when getting out of scope, so you cannot forget it.

I changed so it directly writes to one viewBuilder instead of instantiating many Builder object and copying.

We typically define those as static strings.

Please look at arangod/RestHandler/RestVocbaseBaseHandler.cpp and arangod/RestHandler/RestVocbaseBaseHandler.h
And add the Schema path there.

Just for the sake of completeness:

1. Can you make one of these two graphs hold two Relations e.g. add EDGE_CONTAINS, COLLECTION_PRODUCTS, COLLECTION_MATERIALS to the GRAPH_MANUFACTURE.
   So we can see the API can handle multiple edge collections, and handle overlapping vertex collections.

2. I think I have not talked about orphan collections yet.
   A graph can have Vertex collections that do not have any connected Edge Collection. They are added in the third parameter which is an Array. Like this:
   gm._create("myGraph", [gm._relation("e", "v","v")], ["orphan"]);
   It should be visible as Orphans in the GraphSchema.
   And more importantly the orphan needs to be part of the Collection list when asked for the Graph Schema.

Refactored the code:
(1) GraphSchema now has the attribute orphans and orphan collections are also shown in the collections list.

(2) Added the case where a graph has 2 relations, from and to has 2 documents, and orphans has 2 documents.

(3) Added assertions for the case of (3).

(4) This line ensures that no duplicated collections are shown.

This is okay, but not required.
Dropping the collection will drop all Indexes

Good approach, but this does not cover the Indexes or Graphs.

I would suggest the following:
Create a function "tearDown" in Line 59.
Which takes the full implementation of "tearDownAll" below (that one is good and complete!)
Then at this place call tearDown();
And below you set:

tearDownAll: tearDown
This way you need to implement the cleanup only once.
And the setup makes sure nothing is left from any previously failed test.

Ah, great idea! I changed.

For the following three tests:

test_InvalidSampleNumValues_ShouldReturn400
test_InvalidExampleNumValues_ShouldReturn400
test_ExampleNumGreaterThanSampleNum_ShouldReturn400

They only test the base "schema" endpoint.
I would like to make sure that collection, graph and view are also following this approach.

Therefore can you add another loop in those tests

path = ["", "/collection/products", "/graph/purchaseHistory", "/view/descView"]
path.forEach(p => {
const doc = arango.GET_RAW(api + path + param);
<< your code here >>
})

Just make sure collection / graph / view exists and I did not add a typo in the suggestion.

Sorry this also goes for:

test_ExampleNumZero_ShouldReturnNoExamples
test_SampleNumZero_ShouldReturn400

I wasn't aware of that! I added forEach for all the parameter validation tests not only your suggestions.

The code is correct.

This is not a strict suggestion, just please check if this makes sense or not.

VelocyPack allows for a keep Method.
From the Tests it works like this:

TEST(CollectionTest, KeepSomeAttributes) {
std::string const value(
"{"foo":"bar","baz":"quux","number":1,"boolean":true,"
""empty":null}");

Parser parser;
parser.parse(value);
Slice s(parser.start());

std::vectorstd::string const toKeep = {"foo", "baz", "empty"};
Builder b = Collection::keep(s, toKeep);
s = b.slice();
ASSERT_TRUE(s.isObject());
ASSERT_EQ(3U, s.length());

ASSERT_TRUE(s.hasKey("foo"));
ASSERT_EQ("bar", s.get("foo").copyString());

ASSERT_TRUE(s.hasKey("baz"));
ASSERT_EQ("quux", s.get("baz").copyString());

ASSERT_TRUE(s.hasKey("empty"));
ASSERT_TRUE(s.get("empty").isNull());

ASSERT_FALSE(s.hasKey("number"));
ASSERT_FALSE(s.hasKey("boolean"));
ASSERT_FALSE(s.hasKey("quetzalcoatl"));
}

It should help to keep exact attributes.
Mabye that is easier to use for you?

Convenient method! I changed the code. I thought I could use it for edgeDefinition but it is not a json object so I couldn't use it.

I would like to get away without implementing this query here.
This functionality should be provided by the GraphManager.
I know it does not right now.
But I think it would be easy to add there.

It has an implementation of:
ResultT<std::unique_ptr> lookupGraphByName(
std::string const& name) const;

  Which gives us one Graph Object.
  
  And   Result readGraphs(velocypack::Builder& builder) const;

Which gives us all Graphs in JSON format.

Could you check if you can implement a lookupAllGraphs method?

e.g. ResultT<std::vector<std::unique_ptr>> lookupAllGraphs() const;
By combining the two?
I am okay if the GraphManager would need to run an AQL inside that function.

But this would encapsulate the logic into that one Class only, and it does not leak the information on where Graphs are stored and in which format to any other files.

Refactored the code:

1. Implemented lookupAllGraphs() in GraphManager class.

2. getGraphAndCollections() takes Graph& graph and calls graph.edgeDefinitions() to populate the builder object.

3. getAllGraphsAndCollections() calls _graphManager.lookupAllGraphs() to obtain vector<unique_ptr<Graph>>, and then calls getGraphAndCollections().

Can you use the graphManager lookepGraphByName here?

Like this query 👍

The module name is to describe where the call came from.
So this should be the name of the Schema endpoint