Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-297

Ensure Consistent Behavior Over Deleted Elements

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.1-incubating
    • Fix Version/s: 3.1.0-incubating
    • Component/s: test-suite
    • Labels:

      Description

      Better define the expected behavior when accessing deleted elements. Return empty? How about the label? What about other attributes like in/out vertices, etc? Throw exceptions?

      from Matthias Broecheler

        Activity

        Hide
        okram Marko A. Rodriguez added a comment -

        We probably should have a:

        Exceptions.elementAlreadyDeleted(Class,Object id)

        Show
        okram Marko A. Rodriguez added a comment - We probably should have a: Exceptions.elementAlreadyDeleted(Class,Object id)
        Hide
        mbroecheler Matthias Broecheler added a comment -

        VertexTest.shouldSupportIdempotentRemoval is another example in addition to the property access on deleted edges, vertexproperties, and vertices

        Show
        mbroecheler Matthias Broecheler added a comment - VertexTest.shouldSupportIdempotentRemoval is another example in addition to the property access on deleted edges, vertexproperties, and vertices
        Hide
        mbroecheler Matthias Broecheler added a comment -

        Affected tests:
        EdgeTest.shouldReturnEmptyPropertyIfEdgeWasRemoved
        VertexPropertyTest.shouldReturnEmptyPropertyIfEdgeWasRemoved

        Show
        mbroecheler Matthias Broecheler added a comment - Affected tests: EdgeTest.shouldReturnEmptyPropertyIfEdgeWasRemoved VertexPropertyTest.shouldReturnEmptyPropertyIfEdgeWasRemoved
        Hide
        spmallette stephen mallette added a comment -

        I feel like this one is starting to get out of control. Keeping track of whether or not something is "deleted" kinda stinks in the implementations. I'm also not so sure that the Neo4j implementation of this will hold water under scrutiny (even though the tests are passing).

        Titan OptOut of some of the tests already and I think that will likely have to OptOut of more of them if I continue on this path.

        Matthias Broecheler with respect to your OptOut you wrote:

        > Titan cannot currently throw an exception on access to removed relations due to internal use.

        So, what does Titan end up doing if you call property(k) on a removed element? Does it actually return data? what about property(k,v)? how does that behave on a removed element?

        Show
        spmallette stephen mallette added a comment - I feel like this one is starting to get out of control. Keeping track of whether or not something is "deleted" kinda stinks in the implementations. I'm also not so sure that the Neo4j implementation of this will hold water under scrutiny (even though the tests are passing). Titan OptOut of some of the tests already and I think that will likely have to OptOut of more of them if I continue on this path. Matthias Broecheler with respect to your OptOut you wrote: > Titan cannot currently throw an exception on access to removed relations due to internal use. So, what does Titan end up doing if you call property(k) on a removed element? Does it actually return data? what about property(k,v) ? how does that behave on a removed element?
        Hide
        mbroecheler Matthias Broecheler added a comment -

        If the removed element is an edge or a property, it will return the value. If it is a vertex the property won't be there anymore. This seems inconsistent but is needed internally because I need to read the edge/property property values when deleting it from the storage backend - hence I cannot throw an exception or I would have to implement special internal access methods.
        As for mutating a deleted element - that will throw an exception.

        Show
        mbroecheler Matthias Broecheler added a comment - If the removed element is an edge or a property, it will return the value. If it is a vertex the property won't be there anymore. This seems inconsistent but is needed internally because I need to read the edge/property property values when deleting it from the storage backend - hence I cannot throw an exception or I would have to implement special internal access methods. As for mutating a deleted element - that will throw an exception.
        Hide
        okram Marko A. Rodriguez added a comment -

        Where are we with this? I don't like how Neo4j and TinkerGraph now have deleted booleans. I don't think this is sound in Neo4j and just "wasted space" in TinkerGraph. In TinkerGraph, we can always do a graph.vertices.containsKey(id) to see if the vertex has been deleted, but to do that on every read/write to a TinkerElement seems wildly expensive.

        Show
        okram Marko A. Rodriguez added a comment - Where are we with this? I don't like how Neo4j and TinkerGraph now have deleted booleans. I don't think this is sound in Neo4j and just "wasted space" in TinkerGraph. In TinkerGraph, we can always do a graph.vertices.containsKey(id) to see if the vertex has been deleted, but to do that on every read/write to a TinkerElement seems wildly expensive.
        Hide
        spmallette stephen mallette added a comment -

        given TINKERPOP3-430 and the separate behaviors of the implementations, i wonder if deleted item consistency doesn't become a Feature - in this way the tests can adapt as needed. Of course, if we do a Feature, then writing database agnostic code around removal will require feature checks which is not as nice. Perhaps that is a small sacrifice.

        Show
        spmallette stephen mallette added a comment - given TINKERPOP3-430 and the separate behaviors of the implementations, i wonder if deleted item consistency doesn't become a Feature - in this way the tests can adapt as needed. Of course, if we do a Feature , then writing database agnostic code around removal will require feature checks which is not as nice. Perhaps that is a small sacrifice.
        Hide
        spmallette stephen mallette added a comment -

        Just reverted a first attempt at using "features" for this issue. The test cases got complicated and I'm not so sure that what i was doing actually accomplished what I wanted. This is starting to feel like there is too much disparity among the various implementations to make this completely consistent. Maybe we just remove the booleans and remove all the of the related tests - this bit of consistency may be out of reach.

        Show
        spmallette stephen mallette added a comment - Just reverted a first attempt at using "features" for this issue. The test cases got complicated and I'm not so sure that what i was doing actually accomplished what I wanted. This is starting to feel like there is too much disparity among the various implementations to make this completely consistent. Maybe we just remove the booleans and remove all the of the related tests - this bit of consistency may be out of reach.
        Hide
        spmallette stephen mallette added a comment -

        Deprecated Element.Exceptions.elementAlreadyRemoved and removed test enforcement for consistency in this area as it is clear that maintaining consistency across all the graph implementations is too difficult given the great disparity in how "deleted elements" are treated. It also introduce a fair bit of complexity to the core implementations to maintain the notion of "deleted", so falling back to rely on each graph's sensibilities in this area should simplify code a fair deal. As it stand now, implementers may choose to deal with access to deleted elements in whatever fashion best suits their graph.

        This issue is "breaking" because of the removal of the following tests (which may affect implementer's @OptOut annotations:

        • VertexPropertyTest.ExceptionConsistencyWhenVertexPropertyRemovedTest
        • EdgeTest.ExceptionConsistencyWhenVertexPropertyRemovedTest
        • VertexTest.ExceptionConsistencyWhenVertexRemovedTest

        It is also considered "breaking" In that implementers should review usage of Element.Exceptions.elementAlreadyRemoved. If the above tests were passing and they wish to keep this capability then they should consider throwing their own exception as this method will be removed at some point in the future. Alternatively, they could simply remove code that "tracks" deletion and treat it in some other way.

        For Neo4jGraph this meant completely falling back to using Neo4j handling of "deletion" - there are no checks to try to wrap up their internal handling of such things. For TinkerGraph this meant maintaining state of deletion and using it consistently to prevent mutation of a deleted element.

        Show
        spmallette stephen mallette added a comment - Deprecated Element.Exceptions.elementAlreadyRemoved and removed test enforcement for consistency in this area as it is clear that maintaining consistency across all the graph implementations is too difficult given the great disparity in how "deleted elements" are treated. It also introduce a fair bit of complexity to the core implementations to maintain the notion of "deleted", so falling back to rely on each graph's sensibilities in this area should simplify code a fair deal. As it stand now, implementers may choose to deal with access to deleted elements in whatever fashion best suits their graph. This issue is "breaking" because of the removal of the following tests (which may affect implementer's @OptOut annotations: VertexPropertyTest.ExceptionConsistencyWhenVertexPropertyRemovedTest EdgeTest.ExceptionConsistencyWhenVertexPropertyRemovedTest VertexTest.ExceptionConsistencyWhenVertexRemovedTest It is also considered "breaking" In that implementers should review usage of Element.Exceptions.elementAlreadyRemoved . If the above tests were passing and they wish to keep this capability then they should consider throwing their own exception as this method will be removed at some point in the future. Alternatively, they could simply remove code that "tracks" deletion and treat it in some other way. For Neo4jGraph this meant completely falling back to using Neo4j handling of "deletion" - there are no checks to try to wrap up their internal handling of such things. For TinkerGraph this meant maintaining state of deletion and using it consistently to prevent mutation of a deleted element.

          People

          • Assignee:
            spmallette stephen mallette
            Reporter:
            spmallette stephen mallette
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development