pietermartin / sqlg

TinkerPop graph over sql
MIT License
242 stars 51 forks source link

Stale edge properties after update? #31

Closed johnrevill closed 2 years ago

johnrevill commented 8 years ago

Hello.

I was just testing against a local Postgresql DB (version 9.5 on Linux/Ubuntu 14.04) using the 1.1.0RC2 version from maven (http://mvnrepository.com/artifact/org.umlg/sqlg-postgres/1.1.0.RC2) and

It looks like if there's two different Edge objects that refer to the same actual edge, then updating properties on one of those Edge objects is not reflected in the other Edge property. Not too sure if this is a bug, or a misunderstanding on my part.

The situation is captured in the rough unit test below. It fails on the very last assertion as the "anotherKey" property was not present on one of the Edge objects. At this point, the property is actually both in the DB and present in the other Edge object.

The same situation doesn't seem to affect properties on two Vertex objects referring to the same actual vertex.

Not sure if it helps, but it looks like the condition in the SqlgEdge.load() method (https://github.com/pietermartin/sqlg/blob/1.1.0.RC2/sqlg-core/src/main/java/org/umlg/sqlg/structure/SqlgEdge.java#L230) is finding that the edge's properties are not empty and just considering the edge's properties as already having been loaded and up to date.

package com.centient.commongraph;

import static org.junit.Assert.assertEquals;

import org.apache.commons.configuration.BaseConfiguration;
import org.apache.commons.configuration.Configuration;
import org.apache.tinkerpop.gremlin.structure.Edge;
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.umlg.sqlg.structure.SqlgGraph;

public class EdgeTest {
  private SqlgGraph g;

  @Before
  public void begin() {
    final Configuration conf = new BaseConfiguration();
    conf.addProperty("jdbc.url", "jdbc:postgresql://127.0.0.1:5433/gdbtest");
    conf.addProperty("jdbc.username", "postgres");
    conf.addProperty("jdbc.password", "postgres");

    this.g = SqlgGraph.open(conf);

    // Quick and dirty clear of test DB
    this.g.traversal().E().drop().iterate();
    this.g.traversal().V().drop().iterate();
    this.g.tx().commit();
  }

  @After
  public void after() throws Exception {
    if(g != null) {
      g.close();
    }
  }

  @Test
  public void edgeUpdate() {
    Vertex a = g.addVertex("A");
    Vertex b = g.addVertex("B");
    Edge a2b = a.addEdge("a2b", b);
    a2b.property("someKey", "someValue");

    Edge found_a2b = g.traversal().E().has("someKey", "someValue").next();
    found_a2b.property("anotherKey", "anotherValue");

    g.tx().commit();

    assertEquals("someValue", found_a2b.property("someKey").value());
    assertEquals("anotherValue", found_a2b.property("anotherKey").value());
    assertEquals("someValue", a2b.property("someKey").value());
    assertEquals("anotherValue", a2b.property("anotherKey").value());
  }
}

Cheers, John

pietermartin commented 8 years ago

Hi,

It is only sort of a bug seeing as that feature is not implemented for edges.

For vertices a thread local vertex cache is used to keep them synchronized but this is not implemented for edges.

TinkerPop itself does not have strong semantics around transactions nor the state of 'stale' objects.

I'll add this ticket on the //TODO list.

johnrevill commented 8 years ago

I see, that makes sense. Thanks for your clarity on it.

pietermartin commented 2 years ago

I am removing the vertex transaction scoped cache and will not be doing one for edges. It causes subtle bugs and I am a fan of rule #1 of caches. i.e. 'don't cache'