Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark deleted entities as deleted in the string serialization #6354

Merged
merged 11 commits into from Feb 12, 2016

Conversation

Mats-SX
Copy link
Contributor

@Mats-SX Mats-SX commented Feb 4, 2016

Builds on #6352.
Supersedes #6329.

@oskarhane
Copy link
Member

lgtm from a consumers pov

@@ -82,14 +83,30 @@
import static org.neo4j.test.mocking.GraphMock.relationship;
import static org.neo4j.test.mocking.Properties.properties;

public class ExecutionResultSerializerTest
public class ExecutionResultSerializerTest extends TxStateCheckerTestSupport
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests covering the nested cases?

MATCH (n) DELETE (n) RETURN [n, {someKey: n}]

Also, I'm still unclear on the use case here - what is the remote client supposed to do with the 'isDeleted' flag? Similar for the id that gets returned, the use case seems unclear. Since 99% applications in the wild use labels/properties for identity, what are they supposed to do with that lone id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id gets returned I guess because it is the only thing that we can return. The deleted flag could be checked in order for the client to not get exceptions when trying to lookup properties of the deleted entities. Also, it differentiates the returned entities from non-deleted, property-less dittos. Returning the node without properties (if it had any) would be bad. Returning with properties is not possible.

@Mats-SX
Copy link
Contributor Author

Mats-SX commented Feb 10, 2016

@jakewins Added a smoke test for nesting deleted entities. Good catch.

value.asInstanceOf[Relationship].getType.name()

def compute(value: Any, m: ExecutionContext)(implicit state: QueryState): String =
value match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use CastSupport.castOrFail here?

When returning labels, properties, or relationship types of deleted entities, we
now throw exceptions.

Cleaned up the `type()` function.
Nodes and relationships that are deleted and returned in the same query
will not get their properties serialized, but instead will get a boolean
field 'deleted' set to true.

This commit solves both the 'graph' and the 'row' serializations.
The serialization format `rest` will now be able to correctly return deleted
nodes and relationships.
The row format now will print a sibling 'meta' array, containing node and
relationship ids, the respective type, and the 'deleted' marker field.
@Mats-SX Mats-SX force-pushed the 3.0-deleted-entities-serialization2 branch from 5dfb605 to ac23b4d Compare February 12, 2016 09:25
@Mats-SX
Copy link
Contributor Author

Mats-SX commented Feb 12, 2016

@systay Dealt with your comments, thank you.

systay added a commit that referenced this pull request Feb 12, 2016
…ion2

Mark deleted entities as deleted in the string serialization
@systay systay merged commit 1c6e22f into neo4j:3.0 Feb 12, 2016
@davidegrohmann davidegrohmann deleted the 3.0-deleted-entities-serialization2 branch February 15, 2016 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants