Create non-existent instances #6

Merged
merged 1 commit into from Mar 7, 2014

Conversation

Projects
None yet
5 participants
@agonzalezro
Contributor

agonzalezro commented Feb 27, 2014

It's possible that the object related with the cache model that we are
trying to access doesn't exist yet.

If we had created our caching model with create=True, example:

cache_relation(User.settings, create=True)

the instance for User.settings is going to be created in caste that it
was not found.

Added some spaces to be Flake8 compliant.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Feb 27, 2014

Contributor

I completely understand how useful this would be. However, this dramatically changes the API and expectations of what a "get" should do (ie. it should never do a write!) and feel very very strongly this should not be a default behaviour.

Also this introduces/inherits subtle transaction issues from get_or_create

Contributor

lamby commented Feb 27, 2014

I completely understand how useful this would be. However, this dramatically changes the API and expectations of what a "get" should do (ie. it should never do a write!) and feel very very strongly this should not be a default behaviour.

Also this introduces/inherits subtle transaction issues from get_or_create

@leepa

This comment has been minimized.

Show comment
Hide comment
@leepa

leepa Feb 27, 2014

Consider creating get_or_create_instance()?

leepa commented Feb 27, 2014

Consider creating get_or_create_instance()?

@agonzalezro

This comment has been minimized.

Show comment
Hide comment
@agonzalezro

agonzalezro Feb 27, 2014

Contributor

I completely understand your concerns @lamby. I didn't think at all in other use-cases non related with ours (my mistake).

I am going to create a function get_or_create_instance that is going to solve our problem and it's not going to change the API.

Please, feel free of taking a look to coming PR and comment if you are happy (or not).

Thanks for your feedback!

Contributor

agonzalezro commented Feb 27, 2014

I completely understand your concerns @lamby. I didn't think at all in other use-cases non related with ours (my mistake).

I am going to create a function get_or_create_instance that is going to solve our problem and it's not going to change the API.

Please, feel free of taking a look to coming PR and comment if you are happy (or not).

Thanks for your feedback!

@giftig

View changes

cache_toolbox/core.py
+ if create:
+ instance, _ = model.objects.get_or_create(pk=pk)
+ else:
+ raise exc

This comment has been minimized.

@giftig

giftig Feb 27, 2014

This obscures the stack trace. Just use raise, and don't bother to name the exception.

@giftig

giftig Feb 27, 2014

This obscures the stack trace. Just use raise, and don't bother to name the exception.

This comment has been minimized.

@agonzalezro

agonzalezro Feb 27, 2014

Contributor

Yep, good point I will change this.

@agonzalezro

agonzalezro Feb 27, 2014

Contributor

Yep, good point I will change this.

@giftig

View changes

cache_toolbox/relation.py
@@ -76,7 +76,9 @@ class Foo(models.Model):
from .core import get_instance, delete_instance
-def cache_relation(descriptor, timeout=None):
+def cache_relation(descriptor, timeout=None, create=False):
+ # If create is True the instance is going to be create if it doesn't exist

This comment has been minimized.

@giftig

giftig Feb 27, 2014

created. And this should be a docstring, not a code comment.

@giftig

giftig Feb 27, 2014

created. And this should be a docstring, not a code comment.

This comment has been minimized.

@agonzalezro

agonzalezro Feb 27, 2014

Contributor

We don't have docstrings with :param: anywhere else. I don't think that we should add it just for this function. Anyway, I will add an example to the module documentation as it's everywhere else.

@agonzalezro

agonzalezro Feb 27, 2014

Contributor

We don't have docstrings with :param: anywhere else. I don't think that we should add it just for this function. Anyway, I will add an example to the module documentation as it's everywhere else.

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Feb 27, 2014

Contributor

Thinking about this a little more, without proper UPSERT support this entire idea is really not very robust. I prefer not to have essentially broken-by-design options (even if disabled by default) as it taints the entire library.

Contributor

lamby commented Feb 27, 2014

Thinking about this a little more, without proper UPSERT support this entire idea is really not very robust. I prefer not to have essentially broken-by-design options (even if disabled by default) as it taints the entire library.

@giftig

View changes

cache_toolbox/core.py
+ except model.DoesNotExist:
+ if not create:
+ raise
+ instance, _ = model.objects.get_or_create(pk=pk)

This comment has been minimized.

@giftig

giftig Feb 27, 2014

You're using objects instead of _default_manager here; why? Also you've just failed to get it and now you're trying to get or create it. You should either just use create here, or you should do something like:

   if create:
       instance, _ = models._default_manager.get_or_create(pk=pk)
   else:
       instance = models._default_manager.get(pk=pk)

Also what happens if you are trying to create a model which has required fields without defaults? You're not providing any defaults and you'd presumably have to take these into the function to allow this to work. I also find it odd to try to create something and provide it with a pk, given that pk is an autoincrement field by default.

@giftig

giftig Feb 27, 2014

You're using objects instead of _default_manager here; why? Also you've just failed to get it and now you're trying to get or create it. You should either just use create here, or you should do something like:

   if create:
       instance, _ = models._default_manager.get_or_create(pk=pk)
   else:
       instance = models._default_manager.get(pk=pk)

Also what happens if you are trying to create a model which has required fields without defaults? You're not providing any defaults and you'd presumably have to take these into the function to allow this to work. I also find it odd to try to create something and provide it with a pk, given that pk is an autoincrement field by default.

This comment has been minimized.

@lamby

lamby Feb 27, 2014

Contributor

given that pk is an autoincrement field by default.

The reason for this generally is so that you can use user_id as your primary key; it thus needs to be deterministic and therefore pk needs to be passed in.

create a model which has required fields without defaults

You have that problem anyway; I'm not sure it's a concern of cache_toolbox.

(Agree with all the rest)

@lamby

lamby Feb 27, 2014

Contributor

given that pk is an autoincrement field by default.

The reason for this generally is so that you can use user_id as your primary key; it thus needs to be deterministic and therefore pk needs to be passed in.

create a model which has required fields without defaults

You have that problem anyway; I'm not sure it's a concern of cache_toolbox.

(Agree with all the rest)

This comment has been minimized.

@lamby

lamby Feb 27, 2014

Contributor

agonzalezro, you've now dropped the using(using) call for get_or_create and added a bunch of useless and distracting whitespace changes everywhere.

Please slow down and take more care/pride.

@lamby

lamby Feb 27, 2014

Contributor

agonzalezro, you've now dropped the using(using) call for get_or_create and added a bunch of useless and distracting whitespace changes everywhere.

Please slow down and take more care/pride.

This comment has been minimized.

@giftig

giftig Feb 27, 2014

@lamby those whitespace changes are to make the module flake8-compliant.

@giftig

giftig Feb 27, 2014

@lamby those whitespace changes are to make the module flake8-compliant.

This comment has been minimized.

@leepa

leepa Feb 27, 2014

The commit message should say that's been done as part of the change.

@leepa

leepa Feb 27, 2014

The commit message should say that's been done as part of the change.

This comment has been minimized.

This comment has been minimized.

@lamby

lamby Feb 27, 2014

Contributor

I don't mind whitespace changes if you can justify them, but they obviously shouldn't be done in the same commit as something important...

@lamby

lamby Feb 27, 2014

Contributor

I don't mind whitespace changes if you can justify them, but they obviously shouldn't be done in the same commit as something important...

@giftig

This comment has been minimized.

Show comment
Hide comment
@giftig

giftig Feb 28, 2014

Also please update commit message to indicate flake8 fixes per @leepa

giftig commented Feb 28, 2014

Also please update commit message to indicate flake8 fixes per @leepa

@giftig

This comment has been minimized.

Show comment
Hide comment
@giftig

giftig Feb 28, 2014

You haven't updated the commit message.

giftig commented Feb 28, 2014

You haven't updated the commit message.

@giftig

View changes

cache_toolbox/relation.py
@@ -94,7 +99,9 @@ def get(self):
except AttributeError:
pass
- instance = get_instance(rel.model, self.pk, timeout)
+ instance = get_instance(
+ rel.model, self.pk, timeout, create=create, defaults=defaults or {}

This comment has been minimized.

@giftig

giftig Feb 28, 2014

Just use defaults=defaults here; you don't need the 'or {}' in both places.

@giftig

giftig Feb 28, 2014

Just use defaults=defaults here; you don't need the 'or {}' in both places.

This comment has been minimized.

@agonzalezro

agonzalezro Feb 28, 2014

Contributor

get_instance can be called from everywhere else, not only from core.py.

@agonzalezro

agonzalezro Feb 28, 2014

Contributor

get_instance can be called from everywhere else, not only from core.py.

This comment has been minimized.

@giftig

giftig Feb 28, 2014

So? You're only using defaults here to pass it into get_instance; let get_instance handle what it should do with it. This should just pass it straight in.

@giftig

giftig Feb 28, 2014

So? You're only using defaults here to pass it into get_instance; let get_instance handle what it should do with it. This should just pass it straight in.

This comment has been minimized.

@agonzalezro

agonzalezro Feb 28, 2014

Contributor

Yeah, I didn't realise that this file was relation.py. Will change.

@agonzalezro

agonzalezro Feb 28, 2014

Contributor

Yeah, I didn't realise that this file was relation.py. Will change.

@giftig

View changes

cache_toolbox/relation.py
@@ -20,12 +20,15 @@ class Foo(models.Model):
related_name='foo',
)
- name = models.CharField(max_length=20)
+ name = models.CharField(max_length=20, create=True)

This comment has been minimized.

@giftig

giftig Feb 28, 2014

create=True shouldn't be here...

@giftig

giftig Feb 28, 2014

create=True shouldn't be here...

@giftig

View changes

cache_toolbox/relation.py
@@ -20,12 +20,15 @@ class Foo(models.Model):
related_name='foo',
)
- name = models.CharField(max_length=20)
+ name = models.CharField(max_length=20, create=True)
cache_relation(User.foo)

This comment has been minimized.

@giftig

giftig Feb 28, 2014

...it should be here

@giftig

giftig Feb 28, 2014

...it should be here

@giftig

This comment has been minimized.

Show comment
Hide comment
@giftig

giftig Feb 28, 2014

You also made some changes to make it meet our coding standards, which you haven't mentioned in the commit messages (the local imports etc. aren't flake8 fixes)

giftig commented Feb 28, 2014

You also made some changes to make it meet our coding standards, which you haven't mentioned in the commit messages (the local imports etc. aren't flake8 fixes)

@giftig

View changes

cache_toolbox/relation.py
@@ -22,10 +22,13 @@ class Foo(models.Model):
name = models.CharField(max_length=20)
- cache_relation(User.foo)
+ cache_relation(User.foo, create=True)

This comment has been minimized.

@giftig

giftig Feb 28, 2014

You should also document the 'defaults' param.

@giftig

giftig Feb 28, 2014

You should also document the 'defaults' param.

@giftig

This comment has been minimized.

Show comment
Hide comment
@giftig

giftig Feb 28, 2014

Mostly looks OK to me now but a couple of minor comments left.

giftig commented Feb 28, 2014

Mostly looks OK to me now but a couple of minor comments left.

Create non-existent instances is create=True
It's possible that the object related with the cache model that we are
trying to access doesn't exist yet.

If we had created our caching model with create=True, example:

    cache_relation(User.settings, create=True)

the instance for User.settings is going to be created in caste that it
was not found.

Added some spaces to be Flake8 compliant.
@giftig

This comment has been minimized.

Show comment
Hide comment
@giftig

giftig Feb 28, 2014

Still haven't mentioned standards fixes as well as flake8 ones in commit message, but meh.

This has a +1 from me.

giftig commented Feb 28, 2014

Still haven't mentioned standards fixes as well as flake8 ones in commit message, but meh.

This has a +1 from me.

@agonzalezro

This comment has been minimized.

Show comment
Hide comment
@agonzalezro

agonzalezro Feb 28, 2014

Contributor

I did it on the commit message time ago @giftig, but the description here doesn't get updated.

I've removed the import changes because that is more related with our coding standards than with this project. If I add some gmg coding standards here I would need to add some tests and documentation though. We should create a ENGDEBT card to do that.

Contributor

agonzalezro commented Feb 28, 2014

I did it on the commit message time ago @giftig, but the description here doesn't get updated.

I've removed the import changes because that is more related with our coding standards than with this project. If I add some gmg coding standards here I would need to add some tests and documentation though. We should create a ENGDEBT card to do that.

@Evolter

This comment has been minimized.

Show comment
Hide comment
@Evolter

Evolter Mar 6, 2014

I wonder about having flag in settings for choosing strategy (get or raise exception and get or create), but this looks fine for me.
+1

Evolter commented Mar 6, 2014

I wonder about having flag in settings for choosing strategy (get or raise exception and get or create), but this looks fine for me.
+1

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Mar 7, 2014

Contributor

^ I didn't see any comment on the unsafe nature of this architecture re. UPSERT

Contributor

lamby commented Mar 7, 2014

^ I didn't see any comment on the unsafe nature of this architecture re. UPSERT

@giftig

This comment has been minimized.

Show comment
Hide comment

giftig commented Mar 7, 2014

Ship it

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Mar 7, 2014

Contributor

Classy comment re-ordering there.

Contributor

lamby commented Mar 7, 2014

Classy comment re-ordering there.

@agonzalezro

This comment has been minimized.

Show comment
Hide comment
@agonzalezro

agonzalezro Mar 7, 2014

Contributor

Sorry @lamby but we need this patchset merged to keep our packages and this synced.

If you really think that UPSERT could be a problem (which wasn't for few years), please provide a PR which introduces a suitable test to demonstrate that it fails, and we would be happy to provide a solution if you won't.

I would be happy of testing that behaviour on this package but the lack of tests make it really difficult. We were using our test suit on playfire to test it, and it seems that it works completely fine.

Contributor

agonzalezro commented Mar 7, 2014

Sorry @lamby but we need this patchset merged to keep our packages and this synced.

If you really think that UPSERT could be a problem (which wasn't for few years), please provide a PR which introduces a suitable test to demonstrate that it fails, and we would be happy to provide a solution if you won't.

I would be happy of testing that behaviour on this package but the lack of tests make it really difficult. We were using our test suit on playfire to test it, and it seems that it works completely fine.

agonzalezro added a commit that referenced this pull request Mar 7, 2014

@agonzalezro agonzalezro merged commit 69576c9 into playfire:master Mar 7, 2014

@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby Mar 7, 2014

Contributor

As I explained previously it is the idea/architecture behind using sparse models that this changeset encourages that is faulty, so not only would it be an abstraction-level violation to test it at the code level, it can demonstrate nothing whatsover of value as we can already deduce such a test will fail.

On a practical level, the test would also be cumbersome in the Django framework as requires simultaneous transactions.

http://lucumr.pocoo.org/2014/2/16/a-case-for-upserts/ should be sufficient background documentation if you are not immediately familiar with this general problem area.

(As a meta comment, it's an obvious non-sequitor to respond to a purely technical point with "no, because $process".)

Contributor

lamby commented Mar 7, 2014

As I explained previously it is the idea/architecture behind using sparse models that this changeset encourages that is faulty, so not only would it be an abstraction-level violation to test it at the code level, it can demonstrate nothing whatsover of value as we can already deduce such a test will fail.

On a practical level, the test would also be cumbersome in the Django framework as requires simultaneous transactions.

http://lucumr.pocoo.org/2014/2/16/a-case-for-upserts/ should be sufficient background documentation if you are not immediately familiar with this general problem area.

(As a meta comment, it's an obvious non-sequitor to respond to a purely technical point with "no, because $process".)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment