Closed dusktreader closed 8 years ago
I understand that one of the benefits of using a graph database is their flexibility. For example, in the case of Titan:db, you are encouraged to define a schema for your data, but it isn't required. However, due to the fact that Goblin is an OGM designed to encourage consistent mapping between the application and database data models, we believe that it is best not to allow this type of property setting by default. Furthermore, Goblin is designed to internally handle property names and does not map user defined property names exactly to the database.
If you want to do this, you can still use goblin, but you will have to handle it by submitting a script manually using connection.execute_query
or the gremlin.GremlinMethod
class. To use the example code you gave me earlier:
from goblin import properties
from goblin import connection
from goblin.models import Vertex
from goblin.models.element import Element
from tornado import gen
from tornado.ioloop import IOLoop
class TestVertex(Vertex):
prop1 = properties.String()
@gen.coroutine
def manual_update(self, key, val, **kwargs):
script = 'v = g.V(vid).next(); v.property(k1, v1); v'
bindings = {'vid': self.id, 'k1': key, 'v1': val}
future = connection.get_future(kwargs)
resp = yield connection.execute_query(
script, bindings=bindings, **kwargs)
msg = yield resp.read()
return Element.deserialize(msg.data[0])
@gen.coroutine
def go():
v = yield TestVertex.create(prop1='A')
print(v)
modified_v = yield v.manual_update('prop2', 'B')
print(modified_v)
print("Manual values: {}".format(modified_v._manual_values))
if __name__ == '__main__':
connection.setup('ws://localhost:8182')
loop = IOLoop.current()
try:
print("start")
loop.run_sync(go)
print("end")
except Exception as err:
print("Caught an errror: {}".format(err))
raise err
finally:
connection.tear_down()
loop.close()
However, I would like to note that this is not ideal for most use cases; it is generally better to update the model definition to reflect the new property.
This might be a deal breaker for us moving forward with Goblin.
I am a bit confused why you would allow setting of 'manual_values' in your class constructor, which saves/syncs with the gdb fine, but do not want to allow them to be set after the instance is constructed.
One of the big 3 reasons that we are looking to adopt a graph database system is the flexibility of adding additional properties on the fly.
Well, I don't want this to be a deal breaker for you guys, because I think as a community we can really push this library forward. How about we make a compromise. If I understand correctly, you want to be able to call:
vertex_instance.update("key_not_from_model", "some_val")
Why don't we allow you to do this, but in a way that makes it very obvious what is happening, something like:
vertex_instance.update(manual_values={"key_not_from_model": "some_val"})
Would that be sufficient to cover your use case?
Btw, thanks for all of the reporting and interest!
Yes, you hit on it pretty exactly. I could work with the update method behaving as you described above, as long as that paradigm is also reflected in the constructor and other methods that could access the 'manual_values'
To further the question, we are looking closely at the TinkerPop 3 stack as a good way to keep our implmentation from being closely coupled to neo4j. While neo4j is a nice product, it's really expensive, and we want to be able to switch to another technology with as little pain as possible if the expense is too much. Having a division between manual values and schema values deviates from the philosophy of neo4j. Are there plans/intentions to make Goblin compatible with the Gremlin regardless of the graph technology being used under the hood?
Just to clarify a bit. The _properties
vs _manual_values
distinction was a design choice implemented in mogwai, and I don't think that it really represents any graph database design philosophy. Instead, I think that it is used precisely due to the flexible nature of graph databases. Because most graph dbs, including Titan, don't require explicit schema definition, having the _manual_values
allows you deserialize elements from the database that include properties not defined in the Goblin models. However, when going the other direction (GoblinModel->GremlinServer) it did not allow you to update the model with properties not explicitly declared. In the end, I think this is because following in the tradition of ORMs (like Django ORM), Goblin (mogwai) wanted to provide a way for the developer to precisely control what properties are permitted with each element type. While this sacrifices some of the flexibility of a graph database (and may go against some design philosophy), I think you could see how people might like this. This of course can be overridden, as I showed with the above script. Furthermore, properties can be added/changed in models freely, but there is not yet any sort of syncing mechanism that can apply anything equivalent to a migration. Elements will simply reflect the model definition at the time of their creation.
In the graph database, there is no distinction between these property type (manual vs. schema), unless of course you have used something like Titan's DSL to define a schema, as currently Goblin does no schema management.
In terms of compatibility with other graphdbs, Goblin should be compatible with any TinkerPop enabled database that supports transactions. In some cases, slight modifications to certain groovy scripts may have to be made.
thanks for all the interest @dusktreader!
I can understand why some users would like to have an enforced schema on their node types. We do need to be able to override this behavior, so supplying manual values expicitly to the update() method could work. It would be nice if there was a way to enable/disable seamless interaction with 'manual_values', but I understand the design decision to limit this. How soon might I expect to see this feature in the dev branch? If it's a ways off, I'll continue to work off of my fork
This is on my to do list. I'll probably get it done in the next day or so.
Ok, you can now pass a kwarg manual_values
to the update
method. Check out cc5306874a4335e2de4ab15e20397135e4ed0b51
Unless you object @dusktreader, I will close this.
close away! Thanks for the feature
Ok, well, accidently clicked the close and comment button. So, closed already, I guess
Perfect. Thanks again!
Instead of erroring out when trying to set a property that is not 'officially' part of the class, I enabled the property to be set using the same mechanism that is used in the constructor.
This allows additional properties outside of those described explicitly in the class to be saved via an update