Remove use of auto increment primary key by default

[gordon]

I think due to the nature of this project, it would be beneficial to avoid using AutoField primary keys by default.

I think the simplest way to move forward would be to use a random integer field to avoid causing headaches for deployed shops since AutoField integers are currently being used.  This would allow for roughly 4 billion orders before needing to upgrade.

Any thoughts?  Would this be something accepted into django-shop core?  This seems like a minimum level of obscurity that every production shop would need to implement to prevent revealing sensitive information about the business?  

[Christopher Glass]

That makes sense. If you have a patch suggestion we would be very grateful - otherwise feel free to open a bug on github so that we don't lose track of this :)

Thanks a lot

- Chris

[Simon Luijk]

Or you could use a perfect hash.
http://djangosnippets.org/snippets/1249/

[Ales Kocjancic]

I also stumbled across this problem many times. 

I would suggest just adding a field to your order model that is unique and generating it with something like Simon posted. It would be great to have such a field in core, however I do not agree with changing the primary key (that would cause too many backwards compatibility problems).

So I think the question should be what kind of field would you like to see in core? An integer? A hash of letters+numbers?

[Christopher Glass]

Well perhaps the fact that it was "many times" makes it a prime
candidate for core :)

Ales, I think not messing around with django's IDs is a good idea - so
perhaps we should just add a perfect hash field to all order objects
and make the slug for Orders be this by default in core? Then people
can change that to whatever they see fit - but it seems this would
cover most bases?

- Chris

[gordon]

Just wondering, what would be the backwards compatibility issue if we use an IntegerField as the primary_key for default models?  Any order that used an AutoIncrement key could easily be migrated with south couldn't it?

[gordon]

On Wednesday, September 26, 2012 12:35:42 PM UTC-4, ChrisGlass wrote:

Well perhaps the fact that it was "many times" makes it a prime
candidate for core :)

Ales, I think not messing around with django's IDs is a good idea - so
perhaps we should just add a perfect hash field to all order objects
and make the slug for Orders be this by default in core? Then people
can change that to whatever they see fit - but it seems this would
cover most bases?

- Chris

The only problem with this is that the order number for previous orders that the customer has placed will no longer be valid.  It could potentially be difficult to provide customer support if the order number the customer gives you doesn't pull up an order.

Which could be a non-issue depending on your stance...

[gordon]

Another option I found:   http://search.cpan.org/~esh/Crypt-Skip32/lib/Crypt/Skip32.pm

I found it from this link: http://stackoverflow.com/questions/513056/which-good-block-encryption-algorithm-has-the-shortest-output which I came across from google.

This would probably have the benefit of not requiring a new field as one could just encrypt on retrieval and then decrypt when storing to the database.

I am not sure of the strength of the algorithm, but it appears to be built for this use case.

[Simon Luijk]

You can encrypt/decrypt with a perfect hash too. But I would not
recommend it. If you start with a short output (4 chars) if you then
get lucky and run out of ids the hash can encode. You will need to
increase the number of chars the hash produces. This alters the output
for all previous ids. It should be stored in a field for this reason
and the same reasons addresses are stored in a field.

I have been using a similar method to what Chris suggested in a
invoicing system for a year or so. I will see if I can push the code
to Github.

[jrief]

+1 for this feature.

This is a little off-topic, but during the unit tests for my ipayment module I had the problem, that each test run started with order.id = 1.

That caused some issues, when the test was invoked successively. A customizable order-identifier would fix this.

[Simon Luijk]

There is not much to my implementation really. The second half of the
save method on the model and then the included util. The model does
have to get saved twice to the database, as the pk is needed in
generating the friendly id.

https://github.com/simonluijk/django-invoice/blob/master/invoice/models.py#L30
https://github.com/simonluijk/django-invoice/blob/master/invoice/utils/friendly_id.py

To save one of the database saves you could look at just generating a
random string. You would have to code for the expected id collision
though. You would also eventually need an index on invoice_id. With
the perfect hash you can convert the hash to a pk and lookup in the
database with that.

Anyway just my thoughts, hope they are helpful!

Simon

[gordon]

To save one of the database saves you could look at just generating a
random string. You would have to code for the expected id collision
though. You would also eventually need an index on invoice_id.

Initially, I favored the idea of hashing but I think I like the idea of random better.  Obviously, this is my personal opinion and it will be up to the project maintainers to decide on the final solution.

However, I wrote a proof of concept the other day that generates a random value and handles collisions within the Field class so that Model definitions do not need to be altered.  

I am going to rewrite it cleanly so that one base class can be used generically for any random field regardless of the random value being an integer, string, binary data, etc.  I would like to present it as an option for this issue before a solution is decided upon if possible.

 

[jrief]

Why not adding a callback function or the possibility to overload a method in class Order which delivers the id.

As default we could use the existing primary key, and if someone wants a "creative" order-identifier –  he shall just use that function/method.

[Christopher Glass]

I like the method on Order solution - so that we provide a sane
default but let people adapt the solution to their need.
As for the "random versus hash" bit it seems like random would work
perhaps a little more "cheaply" since collisions are unlikely to
happen very often if the field is large enough, but a perfect hash
would be equally suitable.

- Chris

[jrief]

But random numbers and hashes still have – even if it is very small – a collision probability.

Also, some vendors might prefer to use digits and consecutive numbers rather than hashes. Since PSP's require a unique key to identify the vendors order, I am not sure if every PSP accepts non-numeric identifiers.

Personally, I also prefer to get an order with, say Order-Number: 784343, rather than something such as Order-Identifier: 889949d46d351b28f6b7e097f3368340

- Jacob

[Christopher Glass]

Fair enough, but the fix for random collisions is rather simple - just
get a random number until it doesn't clash anymore.

What does everyone think is the most common use case?

- Chris

[jrief]

What does everyone think is the most common use case?

Status quo: use the autoincremented primary key from order.id (plus an optional offset).

Now as a big plus, we get the ability to add an offset, say the current year multiplied by 100000. We than add the order.id to that number. This gives consecutive numbers prefixed by the year. Many businesses use that scheme for their invoices.

Keep in mind, that for instance, Austrian law requires consecutive numbers on their invoices, so random order identifiers or hashes are not an option.

- Jacob

[gordon]

Keep in mind, that for instance, Austrian law requires consecutive numbers on their invoices, so random order identifiers or hashes are not an option.

- Jacob

Interesting... I did not know this.

 

I like the method on Order solution - so that we provide a sane
default but let people adapt the solution to their need.

I think this is a good way forward.  However, it requires a little more thought because we need to be able to filter on the methods return value.  Otherwise, we cannot retrieve an order by the hash which defeats the purpose.  The simplest way to overcome this would be to require that the hash be stored on the model.  Then the hash method would just need to return the hash field name, which would default to "pk".

If we want to support on the fly hash adjustments we need two additional methods.  One method that prepares the hash for display to the user and another that prepares the hash for database queries.

 

As for the "random versus hash" bit it seems like random would work
perhaps a little more "cheaply" since collisions are unlikely to
happen very often if the field is large enough, but a perfect hash
would be equally suitable.

- Chris

I have been thinking about this and there seem to be so many opinions about the best way to do this that we should make it easy to change.  I think we should create a new subclass of models.Model that inspects an optional configuration dictionary when creating the ID field.  The model should first check if the model name is a key.  If it is, the value of that key should be used to create the primary key.  If the model name is not a key, the model should check for a default value.  If there is no default value, no primary_key should be created and django will install the default AutoField primary key.

The idea is easy to implement, but the specifics of this would need to be ironed out.  I.e. if the field is loaded from the dotted path string, do we also allow the user to define args and kwargs?

All models defined by shop should inherit from this new class instead of models.Model.

[gordon]

So I thought about this a little more...

If we want to support on the fly hash adjustments we need two additional methods. 

 

 

This would also allow hashes to make use of multiple fields with unique_together.  This would be beneficial if you wanted to prefix the year for example and didn't use the pk in the hash because then the hash could roll over every year.  Eventually when django supports composite primary keys this would be even more useful.

 

 

One method that prepares the hash for display to the user 

 

This method should be pretty straightforward.  The hash is constructed from self... e.g. "return '%s-%s' % (self.year, self.pk)"  This might look like "2012-10348".  

 

and another that prepares the hash for database queries.

 

This method should return the parsed hash as a dictionary that maps fields to the pieces of the parsed hash.  I.e. for my previous example, this method would return {"year": 2012, "pk": 10348}.  Then we can do OrderModel.objects.get(**{"year": 2012, "pk": 10348})

[gordon]

However, I wrote a proof of concept the other day that generates a random value and handles collisions within the Field class so that Model definitions do not need to be altered.  

I am going to rewrite it cleanly so that one base class can be used generically for any random field regardless of the random value being an integer, string, binary data, etc. 

If anyone is interested, the proof of concept is here: https://bitbucket.org/pendletongp/django-randomfields

It still needs some polish, but I think the base is good.  I have implemented a random charfield that supports fixed length and variable length strings with a configurable charset.  I have also implemented integer fields in big, normal, and small variants.  This demonstrates that the base class can be used for multiple data types with minimal work.

Mainly the IntegerIdentifier field needs to be worked on.  I like it now because all of the values are going to be the same length and still remain numeric.  This is useful for something like an order id where the customer must be able to give the value to customer service.  It is helpful to quickly be able to tell if a value was communicated properly by checking its length.

However, I think the class might need to be renamed and a PositiveIntegerIdenifier field should be implemented that displays an integer between 1 and (lower_bound + upper_bound + 1).

[Ales Kocjancic]

Hi gordon, I had a quick look and it seems plenty complicated, however, it does also seem to cover most cases. My opinion is that we definitely need an order_number as CharField, not sure what the default method for generating it should be though. Probably keeping it simple would be best, while at the same time allowing the use of something like the app you proposed.

[gordon pendleton]

On Wed, Oct 10, 2012 at 10:44 AM, Ales Kocjancic <ales.ko...@divio.ch> wrote:

Hi gordon, I had a quick look and it seems plenty complicated, however, it does also seem to cover most cases.

I guess it is somewhat complex.  But that was required so that the field can be used without changing the model class.  Since the field hooks into the model save with contribute_to_class, no code modifications are required at the model level.  Using the random field is now no different than a default model field like charfield or autofield for example.


 

My opinion is that we definitely need an order_number as CharField, not sure what the default method for generating it should be though. Probably keeping it simple would be best, while at the same time allowing the use of something like the app you proposed.

I did not mean to imply that my randomfield app should be used by default in django-shop.  I had just mentioned it earlier so I followed up with a link when I had a simple demo django app together.

I think by default, shop should just use an autofield and make it simple to override the "id" field class via settings for all shop models.  There were too many opinions about the requirements of the default primary key to make a choice that will satisfy everyone.

I like the idea of using a "hash" method on the order like Jacob suggested to return the client facing order identifier... but there would also need to be an inverse operation to retrieve an order by "hash" from the database.

[gordon]

My opinion is that we definitely need an order_number as CharField, not sure what the default method for generating it should be though. Probably keeping it simple would be best, while at the same time allowing the use of something like the app you proposed.

I think by default, shop should just use an autofield and make it simple to override the "id" field class via settings for all shop models.  There were too many opinions about the requirements of the default primary key to make a choice that will satisfy everyone.

What about something like this: https://github.com/thenewguy/django-shop/blob/a2dc36fc284fa3bae48b949fa03b6e5fd448619c/shop/models/base.py

This allows you to define a dict in your settings.py file like so:

SHOP_MODEL_PK_MAP = {

    "Order": "randomfields.fields.integer.RandomSmallIntegerField"

}

SHOP_MODEL_PK_MAP_DEFAULT = "randomfields.fields.string.RandomCharField"

such that the shop's Order model pk would be a RandomSmallIntegerField and the other model pks would be of class RandomCharField.  If the default wasn't specified, django's inner workings would automatically add an autofield.

[jrief]

Hello,

Too complicate and not flexible enough. The Order class can be reconfigured anyway.

I would add a Slug-Field to the Order model. Why Slug? Because a) it must be unique anyway, b) Slug is a well established term in Django and c) when displaying the Orders Detail View, this Slug then simply is the part of the URL.

To the default Order class, we shall add as the Slug generator, a function which simply returns the current primary key. This keeps things simple and backwards compatible.

Everyone who wishes to use a different Order numbering schema, just has to derive from the current Order class and overload this one function.

- Jacob

[gordon]

Too complicate and not flexible enough. The Order class can be reconfigured anyway.

I would add a Slug-Field to the Order model. Why Slug? Because a) it must be unique anyway, b) Slug is a well established term in Django and c) when displaying the Orders Detail View, this Slug then simply is the part of the URL.

To the default Order class, we shall add as the Slug generator, a function which simply returns the current primary key. This keeps things simple and backwards compatible.

Everyone who wishes to use a different Order numbering schema, just has to derive from the current Order class and overload this one function.

- Jacob

There is something to be said for simplicity, but I think SlugField is too simple and not purpose fit.  I think the purpose of the SlugField is to create a SEO friendly/meaningful url.  I don't think an order id fits well as a slug.  Plus you would be storing ints as chars for the default case.  Also, wouldn't this approach require two saves on creation since you cannot know the value of an autofield ahead of time?

It is possible that this issue doesn't need to be fixed.  If you have to override the model to define your slug generator, there isn't any benefit from what we are currently doing because it is just as easy to add a field.  Maybe this should just be left to the site developer.

[Ales Kocjancic]

I like jriefs proposal the most. Maybe not call it slug since that would be confusing, but definitely make it a CharField, and add a generator function that can be easily overridden. Saving the order twice on creation shouldn't be a big issue, so I would go with that solution.

[gordon]

jrief,

I believe I have misunderstood you before.

Is this what you were talking about? https://github.com/thenewguy/django-shop/commit/0a1ab059fa64962810f0558f3de12cb960783526

This allows order subclasses to specify a "slug" field to use instead of "pk" if so desired via a classmethod.  

This code defaults to using "pk" as the "slug" field where slug is a generic term, not referring to models.SlugField explicitly.

[Simon Luijk]

IMO the common case is an auto-incrementing id. Maybe just because
most are not bothered enough to choose something else! If that is
going to be the default it seems redundant to also create a charfield
+ unique index just to cater for the few who need something more
exotic. The second most common IMO is an auto incrementing id starting
at an offset.

We could look at making it easier to override without breaking
existing code. I was thinking about implement it similar to Model.pk
in Django. https://github.com/django/django/blob/master/django/db/models/base.py#L449
The problem comes when trying to get it to work with the ORM. The only
implementation I came up with involved overriding all the queryset
methods. All the code to make the pk attribute work as it does is
dotted throughout the db related code in Django. Hmm...

[gordon]

On Thursday, October 11, 2012 4:14:24 PM UTC-4, Simon Luijk wrote:

IMO the common case is an auto-incrementing id. Maybe just because
most are not bothered enough to choose something else! If that is
going to be the default it seems redundant to also create a charfield
+ unique index just to cater for the few who need something more
exotic. The second most common IMO is an auto incrementing id starting
at an offset.

We could look at making it easier to override without breaking
existing code. I was thinking about implement it similar to Model.pk
in Django. https://github.com/django/django/blob/master/django/db/models/base.py#L449
The problem comes when trying to get it to work with the ORM. The only
implementation I came up with involved overriding all the queryset
methods. All the code to make the pk attribute work as it does is
dotted throughout the db related code in Django. Hmm...

Unless I am overlooking something, my changes support that without much modification to the existing api.  I make use of the class based views automatic slug reversing machinery.  https://github.com/thenewguy/django-shop/commit/0a1ab059fa64962810f0558f3de12cb960783526 

By default, my changes use the "pk" field as is.  However, it makes no assumptions about the type of data contained in the pk field.  If the user subclasses the order model and changes the "pk" field type, the order should use that new field class as the order identifier.  

Also, it is not required that the "pk" field be used as the order identifier.  If the user wants to create a new unique field "foo", he can mark that field as the identifier by overriding the "get_slug_field" method of his order to return "foo".  

The only requirement is that "foo" be a model field that can be queried by the orm.  So it could be the primary key or some arbitrary unique field.

I have not extensively tested the changes, but this approach appears to powerful and simple.

[Simon Luijk]

The changes you suggest set an attribute on Order that points to a
model field. Which is the same as what I linked to in Django source
code. But that is only half the story. The difficultly is getting that
property to behave like a model field in the orm. What happens if you
do: Order.objects.get(slug=123) ? My opinion is that that should work.
What do others think? I also don't think slug is the right name. My
suggestion would be `invoice_id` or `order_id`

[gordon]

The changes you suggest set an attribute on Order that points to a
model field. Which is the same as what I linked to in Django source
code. But that is only half the story. The difficultly is getting that
property to behave like a model field in the orm. What happens if you
do: Order.objects.get(slug=123) ? My opinion is that that should work.
What do others think? I also don't think slug is the right name. My
suggestion would be `invoice_id` or `order_id`

You would query it like this:

 Order.objects.get(**{Order.get_slug_field(): 123})  

We could make that easier by adding a get_from_slug method on the manager.

I went with the term "slug" because class based views use this verbage.

[Simon Luijk]

IMHO the more we keep to an existing common API the better.

[gordon]

IMHO the more we keep to an existing common API the better.

Can you explain what you mean by that?  

Currently, the shop api assumes that you are talking about the "pk" when referencing orders.  For this case, nothing changes.  You can still refer to the "slug" as the pk.  So any existing code that uses something like:

 

Order.objects.get(pk=123)

would continue to work by default.  This would only stop working if you changed the slug to refer to a different field, which is not currently supported... so there is no backwards compatibility issues anyways.  

The shop api assumes that you use Order.get_absolute_url to generate your reference urls.  If you do this, nothing changes in the api. 

Also, I believe this would be the closest thing to an existing and common api because this is how django does it:  see https://github.com/django/django/blob/master/django/views/generic/detail.py#L42

I basically only added changes to make the Order model work with the slug mechanism of the class based view we are already using.

[Simon Luijk]

On 13 October 2012 22:14, gordon <wgor...@gmail.com> wrote:
>>
>> IMHO the more we keep to an existing common API the better.
>
>
>
> Can you explain what you mean by that?

I was referring to the get_from_slug method.

> Currently, the shop api assumes that you are talking about the "pk" when
> referencing orders. For this case, nothing changes. You can still refer to
> the "slug" as the pk. So any existing code that uses something like:
>
>
> Order.objects.get(pk=123)
>
>
> would continue to work by default. This would only stop working if you
> changed the slug to refer to a different field, which is not currently
> supported... so there is no backwards compatibility issues anyways.
>
>
> The shop api assumes that you use Order.get_absolute_url to generate your
> reference urls. If you do this, nothing changes in the api.
>
>
> Also, I believe this would be the closest thing to an existing and common
> api because this is how django does it: see
> https://github.com/django/django/blob/master/django/views/generic/detail.py#L42
>
> I basically only added changes to make the Order model work with the slug
> mechanism of the class based view we are already using.

My concern is with plugins. If we go this route it would not be long
before we have apps called "shop-orderid-hash"... These apps would
have to set a model field to store the invoice_id and update the
reference in get_slug_field. As soon as they do this any code using
.get(pk=123) would start returning bad results. To get around this
django-SHOP and plugins would have to use a custom method to get the
order. What I'm suggesting is to continue using the django orm so they
would only have to change .get(pk=123) to .get(order_id=123)

Here is a quick hack to illustrate the concept:
https://github.com/simonluijk/django-shop/commits/orderid/

[gordon]

Here is a quick hack to illustrate the concept:
https://github.com/simonluijk/django-shop/commits/orderid/

Your approach is basically an extension of what I was thinking with syntactic sugar around ".get(**{Order.get_slug_field(): 123})".  I think it would be beneficial to be able rely on a constant kwarg to query on like you propose.  

Minor thought... maybe we could use "oi" or "oid" instead of "order_id" though?  This would keep it short like "pk" and we wouldn't take up a nice field name on the model.  My preference would be "oi" for order identifier like "pk" is for primary key.

[gordon]

field name on the model.  My preference would be "oi" for order identifier like "pk" is for primary key.

Or maybe "ui" for unique identifier.  I think it should be two letters at any rate. 

[Simon Luijk]

On 14 October 2012 01:11, gordon <wgor...@gmail.com> wrote:
>
>> Here is a quick hack to illustrate the concept:
>> https://github.com/simonluijk/django-shop/commits/orderid/
>
>
> Your approach is basically an extension of what I was thinking with
> syntactic sugar around ".get(**{Order.get_slug_field(): 123})". I think it
> would be beneficial to be able rely on a constant kwarg to query on like you
> propose.

I can see how it looks like syntactic sugar in it's current form. With
a little re-factoring we could support all the orm features we have
grown accustomed too. e.g. Order.objects.order_by('order_id') or
Order.objects.filter(order_id__in=[4, 7, 3])

There are allot of features to the django orm, most of them probably
not needed. It would probably be best to only add them when needed.
For now I think we should support the `get` and `filter` methods.

> Minor thought... maybe we could use "oi" or "oid" instead of "order_id"
> though? This would keep it short like "pk" and we wouldn't take up a nice
> field name on the model. My preference would be "oi" for order identifier
> like "pk" is for primary key.

Yes we could change the name. Short is normally always better. This
would be the public name though and the custom id field, defined in
plugins or shop instances, would be behind the scenes.

[Ales Kocjancic]

What is wrong by simply having the order number as a field, and a method that sets it on save?

[gordon pendleton]

Wouldn't that prevent subclasses from changing the field type and requirements?  Or do you mean by requiring a field named "order_foo" to be on the order?

https://docs.djangoproject.com/en/dev/topics/db/models/#field-name-hiding-is-not-permitted

[gordon pendleton]

Sorry for the top post.  I am not around a computer and my phone did it by default.

[Simon Luijk]

On 14 October 2012 20:06, Ales Kocjancic <ales.ko...@divio.ch> wrote:
> What is wrong by simply having the order number as a field, and a method
> that sets it on save?

If the default case is to set it to pk, it would add an extra database save.

[Ales Kocjancic]

I'm ok with that. It makes everything much simpler, and if you really want something special you can anyway overwrite the whole model and views.

[gordon]

I'm ok with that. It makes everything much simpler, and if you really want something special you can anyway overwrite the whole model and views.

The current situation if someone wants something "special" is that they override the model and/or the views.  Now, after this update, if someone wants something special, they will still have to override the model and/or the views, but they will also need to rewrite the order base class to override the CharField used if something like the max_length needs to be adjusted.  To me, it sounds like the current situation is just as good and maybe should be left as is.  Maybe we should just remove any occurrences (if there are any... I think I remember seeing one or two but it may have been in a plugin) that expect the primary key to be numeric so that the primary key can be overridden with a CharField.

I initially brought up this issue because every online store I've shopped at appears to use order identifiers that hide sensitive information about the rate and number of orders processed.  I figured that doing this by default would be in everyone's best interest.  However, this email chain brought out that in some countries, this is illegal... e.g. Australia, and it appears the default case will support this out of the box.  It also seems like everyone has a different requirement for an order number, so a trivial solution seems to be inadequate.

At any rate, I feel like I am just creating friction and the order model is inevitably going to have a hard-coded CharField.  I don't want to be argumentative, so I look forward to the final decision.