linkyndy / remodel

Very simple yet powerful and extensible Object Document Mapper for RethinkDB, written in Python.
MIT License
193 stars 29 forks source link

Saving an object removes all links to its related objects #51

Open catroot opened 8 years ago

catroot commented 8 years ago

I trying to repeat a demo tutorial with code:

from remodel.models import Model
from remodel.helpers import create_tables, create_indexes

class Products(Model):
    has_many=('Code',)
    has_one=('Value',)

class Code(Model):
    has_one=('Value',)

class Value(Model):
    belongs_to_many=('Product', 'Code',)

create_tables()
create_indexes()
code=Code.create(code='sample')
code['value'].add(Value.create(value=10))

And this code raises error:

Traceback (most recent call last):
  File "models-test.py", line 18, in <module>
    code['value'].add(Value.create(value=10))
AttributeError: 'NoneType' object has no attribute 'add'

Whats wrong with code?

mentat-enki commented 8 years ago

I believe you are looking for: code['value'] = Value.create(value=10). Since the relation is "singular" in nature (due to has_one), you need to assign the Value instance directly, rather than add it to a list as you would with a has_many or has_and_belongs_to_many relation.

If you read the Models page on the wiki, at the top it mentions the scheme used when "inflecting" relation names.

Also, for what it's worth, given a class you can find the generated name for that class via ._table, e.g. Value._table --> 'value'. Also, given an instance of a model, you can discover what relation-names are available for use via instance.fields.related, e.g. with your code code.fields.related --> {'value'}.

Also, take note that belongs_to_many as used in Value isn't a valid relation type. Not sure exactly what you are gunning for there, but perhaps you meant has_and_belongs_to_many

Hope this is helpful.

Gesias commented 8 years ago

You can only use "add" on that table relation if it has a "has_many". Code for example "has_one" Value and add is used for "has_many" situations. See the example again.

class Country(Model):
    has_many = ('City',)

class City(Model):
    belongs_to = ('Country',)

romania = Country.create(name='Romania')
romania['cities'].add(City(name='Timisoara'), City(name='Bucharest'))
print romania['cities'].count() # prints 2
catroot commented 8 years ago

Ok. Got it. Thank you for explaining index mode features.

But what happens after calling .save() on code?

code=Code.create(code='sample')
code['value']=Value.create(value=10)
print(code['value']['value']) # prints 10
code.save()
print(code['value']) # prints None

Where to my ['value'] is gone?

mentat-enki commented 8 years ago

I experimented with your example a bit. Indeed, the way you are using it doesn't seem to be working, and may very well be considered a bug as that pattern of use isn't very far-fetched, however, if you re-arrange your code slightly, you can achieve what you are after. For example (slightly simplified for this example discussion):

from remodel.models import Model
from remodel.helpers import create_tables, create_indexes, drop_tables

class Code(Model):
    has_one=('Value',)

class Value(Model):
    belongs_to=('Code',)

create_tables(); create_indexes()

code=Code.create(code='sample')

# Rather than setting the Value to the Code, as you were doing via:
#     `code['value']=Value.create(value=10)`
# Do the following instead… either:

value = Value.create(value=10, code=code)

# …no save necessary here - create automatically persists the record
print(value['code']) #--> <Code: […id…]>
print(code['value']) #--> <Value: […id…]>

# or you can also do it this way I believe:
value = Value.create(value=10)
value['code'] = code
value.save()

The gist here I think is that the association must be made on the "belonging" object, not the "has-ing" object in the relationship.

I was genuinely surprised that your initial code didn't work as expected, as that pattern of use seems reasonable to me. Maybe @linkyndy can shed some light or agree that what you tried was legit and that what you found is a "bug".

linkyndy commented 8 years ago

Indeed, I've spotted a bug on that part. I wanted to flush related caches, but unfortunately I am also wiping existing descriptors defined on the field handler itself.

I'll start working on a fix for this issue. Thanks for reporting it!

linkyndy commented 5 years ago

Actually, after more thinking on this, I am inclined to say that the current behaviour is the intended one. Let me explain.

In the above example, we are operating on a Code object. When saving this object, we're interested in saving fields for this actual object, not also its related objects. This would raise a few concerns, if considered: what happens if the related object is invalid? Should the whole operation fail (so, even the Code object will not be saved)? What happens if that object is already saved/not saved yet?

I admit I've borrowed a few concerns from Django's relationship model, that is, HasMany and HasAndBelongsToMany relations are being updated on assignment, while HasOne and BelongsTo aren't -- not even when the parent object is saved. This is in contrast with other frameworks, such as Ruby on Rails, where a lot more magic is involved. I've opted for something closer to Django's relationship model because it is more straight-forward and the developer controls operations in their entirety (so, no "magic" saves involved).

With this in mind, the above example can be made "right":

code=Code.create(code='sample')
value=Value.create(value=10) # We need this object to save it after it's being assigned to `code`
code['value']=value # This sets `code_id` on `value`
print(code['value']['value']) # prints 10
value.save() # This ensures `value` is saved with `code_id` set
code.save()
print(code['value']['value']) # prints 10

The difference vs the former example is that previously, value was never saved with code_id set, since this field got set after value was saved (part of its creation). Now, by explicitly saving the value object, we make sure its "ties" to the code object are properly persisted.

This is my view on this specific case, and on remodel's relationship model. Let me know if that makes sense to you as well, and if not, let's discuss potential improvements related to this!