New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for non auto-increment/integer primary keys #92

Closed
mikeywaites opened this Issue Sep 13, 2013 · 9 comments

Comments

Projects
None yet
5 participants
@mikeywaites

Our system makes use of UUID column type in postgres in some places. At the moment FactoryBoy will try to perform a lookup on the primary key to determine the next sequence using the max() function on the primary key column of a model

/factory/alchemy.py", line 38, in _setup_next_sequence
max_pk = session.query(max(pk)).one()[0]

#produces the following sql statement.
SELECT max(foo.id) AS max_1 \nFROM foo' {}

This is obviously not going to work when we have UUID's

Is this something you might consider allowing for?

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Sep 13, 2013

Member

Yes, there have been a few bug reports around this bad feature.
I'm currently looking for a clean way to handle the problem, without including too much magic :)

Member

rbarrois commented Sep 13, 2013

Yes, there have been a few bug reports around this bad feature.
I'm currently looking for a clean way to handle the problem, without including too much magic :)

@mikeywaites

This comment has been minimized.

Show comment
Hide comment
@mikeywaites

mikeywaites Sep 13, 2013

Thanks for getting back to me. I planned on having a stab at it, but as you said its not the most straight forward thing in the world hey!

Could it be a simple as providing a separate base for this use case?? I suppose the functionality is spread across two classes though. Perhaps even just a flag to disable the sequence generation functionality in base.py (forgive me if i got that file wrong) that might be nice and explicit and could just leave the user responsible for handling primary keys in any use case?

Thanks for getting back to me. I planned on having a stab at it, but as you said its not the most straight forward thing in the world hey!

Could it be a simple as providing a separate base for this use case?? I suppose the functionality is spread across two classes though. Perhaps even just a flag to disable the sequence generation functionality in base.py (forgive me if i got that file wrong) that might be nice and explicit and could just leave the user responsible for handling primary keys in any use case?

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Sep 16, 2013

Member

All happens in factory.base.BaseFactory._setup_next_sequence (which is pretty simple) and its overrides (django / sqlalchemy).

The main issue on both sides is that, actually, pks may be non numeric. I guess the simplest way to handle the problem would be to look at introspections tools in the frameworks to detect whether the table's primary key is an AUTO_INCREMENT-like field.

Another option is to simply try to fetch the latest pk value from the database and attempt to convert it into an int().

If SQLAlchemy introspections features make it easy to find the type of the primary key column, this check should be performed before attempting to call max() within the database.

Member

rbarrois commented Sep 16, 2013

All happens in factory.base.BaseFactory._setup_next_sequence (which is pretty simple) and its overrides (django / sqlalchemy).

The main issue on both sides is that, actually, pks may be non numeric. I guess the simplest way to handle the problem would be to look at introspections tools in the frameworks to detect whether the table's primary key is an AUTO_INCREMENT-like field.

Another option is to simply try to fetch the latest pk value from the database and attempt to convert it into an int().

If SQLAlchemy introspections features make it easy to find the type of the primary key column, this check should be performed before attempting to call max() within the database.

@krak3n

This comment has been minimized.

Show comment
Hide comment
@krak3n

krak3n May 14, 2014

Hey Guys

Currently battling with this now on the SQLAlchemy side of things, had a bit of a hack and basically did this:

from sqlalchemy.types import Integer    

@classmethod
def _setup_next_sequence(cls, *args, **kwargs):
    """Compute the next available PK, based on the 'pk' database field."""
    session = cls.FACTORY_SESSION
    model = cls.FACTORY_FOR

    pk = getattr(model, model.__mapper__.primary_key[0].name)

    # Check if the primary key type is an Integer, if it is then we do the normal max
    # stuff else return 1 - we might also need to check BigInteger as well

    if isinstance(pk.type, Integer):
        max_pk = session.query(max(pk)).one()[0]
        if isinstance(max_pk, int):
            return max_pk + 1 if max_pk else 1

    return 1

Seems to do the job.

krak3n commented May 14, 2014

Hey Guys

Currently battling with this now on the SQLAlchemy side of things, had a bit of a hack and basically did this:

from sqlalchemy.types import Integer    

@classmethod
def _setup_next_sequence(cls, *args, **kwargs):
    """Compute the next available PK, based on the 'pk' database field."""
    session = cls.FACTORY_SESSION
    model = cls.FACTORY_FOR

    pk = getattr(model, model.__mapper__.primary_key[0].name)

    # Check if the primary key type is an Integer, if it is then we do the normal max
    # stuff else return 1 - we might also need to check BigInteger as well

    if isinstance(pk.type, Integer):
        max_pk = session.query(max(pk)).one()[0]
        if isinstance(max_pk, int):
            return max_pk + 1 if max_pk else 1

    return 1

Seems to do the job.

@dfee

This comment has been minimized.

Show comment
Hide comment
@dfee

dfee Sep 19, 2014

It's even easier. Just supply __sequence to the factory. It's not heavily documented, though.
http://factoryboy.readthedocs.org/en/latest/reference.html?highlight=reset#forcing-a-sequence-counter

dfee commented Sep 19, 2014

It's even easier. Just supply __sequence to the factory. It's not heavily documented, though.
http://factoryboy.readthedocs.org/en/latest/reference.html?highlight=reset#forcing-a-sequence-counter

@NiklasMM

This comment has been minimized.

Show comment
Hide comment
@NiklasMM

NiklasMM Nov 12, 2014

It's even easier. Just supply __sequence to the factory. It's not heavily documented, though.
http://factoryboy.readthedocs.org/en/latest/reference.html?highlight=reset#forcing-a-sequence-counter

Does this not defeat the purpose of the Sequence altogether? Sure I can provide __sequence to the instances, but then I might as well leave it out.

I think what @krak3n suggested is fine. If you use a Sequence on a non-Integer primary key, you should be responsible to make it work, but it should also not break, like it does now.

It's even easier. Just supply __sequence to the factory. It's not heavily documented, though.
http://factoryboy.readthedocs.org/en/latest/reference.html?highlight=reset#forcing-a-sequence-counter

Does this not defeat the purpose of the Sequence altogether? Sure I can provide __sequence to the instances, but then I might as well leave it out.

I think what @krak3n suggested is fine. If you use a Sequence on a non-Integer primary key, you should be responsible to make it work, but it should also not break, like it does now.

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Nov 12, 2014

Member

@NiklasMM Well, it should "just work" anyway.

The only purpose of this weird magic with automagical sequence initialization is to support the following scenario:

  1. Run a Python script that uses MyFactory.create() a couple of times (with a unique field based on the sequence counter)
  2. Run the same Python script a second time

Without this hack, the Sequence counter will be set to 1 at the beginning of each script run, so both runs would generate objects with the same values for the unique field ; thus conflicting and crashing.

I'm willing to remove this hack : the above scenario isn't a standard use of the library, and the behavior can be easily achieved with custom reset_sequence or create(__sequence=42) calls.

Member

rbarrois commented Nov 12, 2014

@NiklasMM Well, it should "just work" anyway.

The only purpose of this weird magic with automagical sequence initialization is to support the following scenario:

  1. Run a Python script that uses MyFactory.create() a couple of times (with a unique field based on the sequence counter)
  2. Run the same Python script a second time

Without this hack, the Sequence counter will be set to 1 at the beginning of each script run, so both runs would generate objects with the same values for the unique field ; thus conflicting and crashing.

I'm willing to remove this hack : the above scenario isn't a standard use of the library, and the behavior can be easily achieved with custom reset_sequence or create(__sequence=42) calls.

@NiklasMM

This comment has been minimized.

Show comment
Hide comment

+1

rbarrois added a commit that referenced this issue Nov 16, 2014

Remove automagic pk-based sequence setup
Related to issues #78, #92, #103, #111, #153, #170

The default value of all sequences is now 0; the automagic
``_setup_next_sequence`` behavior of Django/SQLAlchemy has been removed.

This feature's only goal was to allow the following scenario:

1. Run a Python script that uses MyFactory.create() a couple of times
   (with a unique field based on the sequence counter)
2. Run the same Python script a second time

Without the magical ``_setup_next_sequence``, the Sequence counter would be set
to 0 at the beginning of each script run, so both runs would generate objects
with the same values for the unique field ; thus conflicting and crashing.

The above behavior having only a very limited use and bringing various
issues (hitting the database on ``build()``, problems with non-integer
or composite primary key columns, ...), it has been removed.

It could still be emulated through custom ``_setup_next_sequence``
methods, or by calling ``MyFactory.reset_sequence()``.
@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Mar 26, 2015

Member

This issue has been fixed in 13d310f, and will be part of the 2.5.0 release.

Member

rbarrois commented Mar 26, 2015

This issue has been fixed in 13d310f, and will be part of the 2.5.0 release.

@rbarrois rbarrois closed this Mar 26, 2015

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