arangodb / arangodb-php

PHP ODM for ArangoDB
https://www.arangodb.com
Apache License 2.0
183 stars 43 forks source link

Fix: The edges api call didnt work anymore. #181

Closed DonMartio closed 9 years ago

DonMartio commented 9 years ago

I don't now whether this never worked or the api changed. I fixed it the second time so i think a pull request is appropriate.

What it did before: /_api/edge/?collection=vertex=&direction=out but this didn't work This is what it does now: /_api/edges/?vertex=&direction=out this works for us.

frankmayer commented 9 years ago

Hi and thanks for the pull request. However, it seems to fail on ArangoDB 2.5.1, which is currently used for testing the PHP client. With what version of ArangoDB have you had this problem (and this fix did work)?

BTW, I just did a quick test locally with ArangoDB 2.5.3 and the tests (including a get edges test, using probably the old API style, still works.

frankmayer commented 9 years ago

So, it seems that this was at some point the way to get edges, but I can't find any API documentation on this now. However, the new way using /edges/... does not only return an array of cursors, but an array of the complete edge documents. This is a bigger change in the way the method works and may break people's code. Either we introduce this change in a new version or in a new method. Any ideas?

DonMartio commented 9 years ago

Hu, ok I misunderstood this function as it seems. When tried to use it, it always gave a list of all edges in the collection. Furthermore I expected the hole 'edge' documents I considered that an error. As you suggested this change would break other peoples code so either we create something new or I look for another way. The documentation about this I found here :https://www.arangodb.com/manuals/2/implementor-manual.pdf on page 36.

jsteemann commented 9 years ago

Summarizing the status quo:

I think the server API methods at /_api/edge never announced or interpreted the direction URL parameter. The methods for at /_api/edge are meant to be CRUD operations for handling single edges (i.e. create an edge, remove an edge, retrieve an update, update an edge). If either a direction or a vertex URL parameter were passed to these methods, they have been ignored (i.e. the GET call will return an array with all edges in the edge collection). It looks like the PHP driver's edges() method calls this API with direction and vertex parameter, suggesting this would restrict the result to the specified values. But it will always return all edges from the collection. It looks like this method worked this way since it was first added.

Then there is also the server API at /_api/edges, which is meant to return arrays of connected edges. This will interpret the direction and vertex URL parameters and would be the way to retrieve a list of connected edges for a given vertex. The PHP driver does not seem to call this at the moment.

frankmayer commented 9 years ago

So, the question is, do we just change this in a patch version or wait for a minor version, then?

jsteemann commented 9 years ago

I have pushed an incompatible change in the devel branch of the PHP driver: https://github.com/arangodb/arangodb-php/commit/b65085c0b993cde1b5c7c2c9a8dc6dcf0dc29b8c

I think (incompatibly) changing the behavior of the driver's edges(), inEdges() and outEdges() methods makes sense here, because these methods did not do what their documentation suggested. Now they do, however, their output format has changed. They now return an array of edge objects instead of an array of edge ids. Additionally, they only return the actually connected edges and not all edges from the collection. The new behavior however is in line with the documentation.

The incompatible change is for devel, so it can be announced properly when the next release branch (2.6) of the driver will be created out of it. 2.5 won't change.

Any contrary thoughts on this?

frankmayer commented 9 years ago

I concur :+1:

DonMartio commented 9 years ago

Great.

jsteemann commented 9 years ago

Ok, then I suggest closing this issue. If it's not a big problem I suggest you go ahead with your patched version in 2.5 and remove the local modifications once the devel branch becomes a 2.6 release. Thanks for the pull request.