Add unique_constraints, a more versatile replacement for django_get_or_create and sqlalchemy_get_or_create #794

rbarrois · 2020-10-11T22:50:16Z

This change deprecates django_get_or_create and sqlalchemy_get_or_create; upgrade helpers are provided through a newly-introduced option renaming helper.

Closes #241: The new unique_constraints feature is more powerful, but supports the exact same cases; a specific case has been added to opt out of lookups for Factory.build() and Factory.stub().
Examples are provided for Django and SQLAlchemy unique constraint declarations.

Closes #69: With the implementation of unique_constraints - and its lazy declaration resolution algorithm, non-constrained declarations will only be resolved once lookups have failed; this avoids creating orphan objects.

Closes #781: The call-time parameters are now explicitly included in the lookup algorithm; they no longer need to be kept attached to the factory class, which leaked instances.

Note: the change includes updated code, additional tests, deprecation warning and migration helpers.

The code responsible for `get_or_create` places a copy of the passed-in params on the factory class itself; which is kept after the instance has been created. Those fields are part of the BuildStep context, and should be fetched from there instead. The issue also occurs in DjangoModelFactory. See #781 for details.

This could be used to ensure that an entry is actually a list.

Through `def _get_renamed_options()`, a specific `FactoryOptions` subclass may now indicate that one of its fields should now use another name, and optionally another syntax. The warning message displays the new format to use when adjusting the option.

rbarrois · 2020-10-11T22:52:56Z

@francoisfreitag if you've got some time, I'd be happy to get another set of eyes on this ;)

Declare unique constraints on field values through this new `class Meta` option. When building a new instance, the parameters will first be resolved for each unique constraint, and a dedicated lookup performed; an instance will only be created if each lookup failed. Declarations are resolved lazily: a non-constraint SubFactory will only be called if each previous constraint lookup failed.

This change deprecates django_get_or_create and sqlalchemy_get_or_create; upgrade helpers are provided throught the option renaming helpers. Closes #241 The new `unique_constraints` feature is more powerful, but supports the exact same cases; a specific case has been added to opt out of lookups for `Factory.build()` and `Factory.stub()`. Examples are provided for Django and SQLAlchemy unique constraint declarations. Closes #69 With the implementation of `unique_constraints` - and its lazy declaration resolution algorithm, non-constrained declarations will only be resolved once lookups have failed; this avoids creating orphan objects. Closes #781 The call-time parameters are now explicitly included in the lookup algorithm; they no longer need to be kept attached to the factory class, which leaked instances.

The variable is a StepBuilder, not a BuildStep...

francoisfreitag

This is awesome ⭐.

It unifies <orm>_get_or_create() under a single option. Having the unique_constraints matching Django unique_together facilitates writing unique_constraints for Django. unique_constraints are more powerful than get_or_create, they grant more controls over the lookup strategy. Removing the junk data generated when resolving the factory declarations is just great.

I’m sure users will be thankful for the very clear migration path. It would be nice to provide a deadline for removing the compatibility schemes for get_or_create. Perhaps for version 4.0?

IMO, the following commits should be squashed. They all have a single purpose, adding the unique_constraints feature. Splitting them was helpful for the review 👍.

Expose memleak in SQLAlchemyModelFactory.
Add an optional normalization to class Meta fields
Provide support for migrating class Meta fields
Add unique_constraints and associated _lookup()
Replace *_get_or_create with unique_constraints.

As far as I’m concerned, get_or_create (or unique_constraints) should not be used in tests. When writing a test, the exact state of the database should be known and the dev should know whether the data is to be created or if it’s already present in the DB and should be retrieved. But users rely on that feature, and getting it an upgrade is likely beneficial to them.

Commit “Expose memleak in SQLAlchemyModelFactory.”

-Expose memleak in SQLAlchemyModelFactory.
-Expose memleak in SQLAlchemyModelFactory

Commit “Replace *_get_or_create with unique_constraints.”:

-Replace *_get_or_create with unique_constraints.
+Replace *_get_or_create with unique_constraints

and

-upgrade helpers are provided throught the
+ upgrade helpers are provided through the

Commits “Rename a StepBuilder instance to step_builder” and “docs: Use .. rubric:: for minor sections. ” could be separate PRs, this one is big enough.

francoisfreitag · 2020-10-13T06:11:00Z

tests/test_django.py

+
+        # Ensure the instance pointed to by the weak reference is no longer available.
+        self.assertIsNone(target_weak())
+


Suggested change

francoisfreitag · 2020-10-13T06:22:13Z

factory/base.py

 class FactoryOptions:
+


Suggested change

francoisfreitag · 2020-10-13T06:22:24Z

factory/base.py

+        self.transform = transform
+
+    def __str__(self):
+        return '%s(%r, %r, tranform=%r)' % (


Suggested change

return '%s(%r, %r, tranform=%r)' % (

return '%s(%r, %r, transform=%r)' % (

francoisfreitag · 2020-10-13T06:29:29Z

tests/test_base.py

+            self.assertEqual(1, len(w))
+            message = str(w[-1].message)


Suggested change

self.assertEqual(1, len(w))

message = str(w[-1].message)

[warn] = w

message = str(warn.message)

francoisfreitag · 2020-10-13T06:31:48Z

docs/internals.rst

+2. That method computes groups of lookups to try:
+
+   - Any call-time parameter that appear in any unique constraint will *always* be included;
+   - The first lookups are performed on the unique constraintss sharing the most parameters


Suggested change

- The first lookups are performed on the unique constraintss sharing the most parameters

- The first lookups are performed on the unique constraints sharing the most parameters

francoisfreitag · 2020-10-13T08:55:48Z

tests/test_django.py


 class MultipleGetOrCreateFieldsTest(django_test.TestCase):
-    def test_one_defined(self):
+    def test_any_field(self):


While this is a better name, the improvement could be made in a separate commit.

francoisfreitag · 2020-10-13T08:55:54Z

tests/test_django.py

@@ -1048,4 +1082,3 @@ class Meta:

        # Ensure the instance pointed to by the weak reference is no longer available.
        self.assertIsNone(target_weak())
-


The improvement could be made in a separate commit.

francoisfreitag · 2020-10-13T09:00:56Z

tests/test_using.py

+        if kwargs == self.keys:
+            return self.instance
+        else:
+            return None


Suggested change

if kwargs == self.keys:

return self.instance

else:

return None

return self.instance if kwargs == self.keys else None

francoisfreitag · 2020-10-13T09:02:33Z

tests/test_using.py


            a = factory.Sequence(lambda n: 'foo_%s' % n)
            b = 2
            c = 3
            d = 4

        o = TestModelFactory()
-        self.assertEqual({}, o._defaults)
+        self.assertEqual(dict(a='foo_0', b=2, c=3, d=4), TestModel.objects.last_lookup)


Suggested change

self.assertEqual(dict(a='foo_0', b=2, c=3, d=4), TestModel.objects.last_lookup)

self.assertEqual({"a": 'foo_0', "b": 2, "c": 3, "d": 4}), TestModel.objects.last_lookup)

francoisfreitag · 2020-10-13T09:03:30Z

tests/test_using.py

@@ -2293,15 +2299,17 @@ def test_full_get_or_create(self):
        class TestModelFactory(factory.django.DjangoModelFactory):


The docstring above this line could be updated to remove the get_or_create mention.

MRigal · 2020-11-02T11:09:52Z

tests/test_using.py

+        self.assertEqual(o1, o4)
+        self.assertEqual(o2, o6)
+        self.assertEqual([o1, o2, o3, o5, o7], LookupGroupTests.store)
+


The current django_get_or_create fails when one of the unique field is a Trait or Maybe. Maybe you could add a test to show that (and in the worst case xfail it)

I don't know what you're talking about; could you point to a code sample where that fails?

Here is a failing test case on the actual implementation. I could adapt it to yours or you might take a look directly #810

aalvrz · 2021-02-08T22:47:38Z

Any updates on this?

rbarrois added 3 commits October 12, 2020 00:42

Add an optional normalization to class Meta fields

11fe368

This could be used to ensure that an entry is actually a list.

rbarrois added Django Feature Improvement SQLAlchemy labels Oct 11, 2020

rbarrois force-pushed the rbarrois/unique-constraints branch from 8ea93ea to 2a87857 Compare October 11, 2020 22:55

rbarrois added 4 commits October 12, 2020 00:55

docs: Use .. rubric:: for minor sections.

772a067

Rename a StepBuilder instance to step_builder

2a87857

The variable is a StepBuilder, not a BuildStep...

rbarrois requested a review from francoisfreitag October 11, 2020 22:56

francoisfreitag mentioned this pull request Oct 13, 2020

FK to existing object is ignored, SubFactory is used instead #571

Closed

francoisfreitag reviewed Oct 13, 2020

View reviewed changes

MRigal reviewed Nov 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unique_constraints, a more versatile replacement for django_get_or_create and sqlalchemy_get_or_create #794

Add unique_constraints, a more versatile replacement for django_get_or_create and sqlalchemy_get_or_create #794

rbarrois commented Oct 11, 2020

rbarrois commented Oct 11, 2020

francoisfreitag left a comment

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

francoisfreitag Oct 13, 2020

MRigal Nov 2, 2020

rbarrois Nov 2, 2020

MRigal Nov 2, 2020

aalvrz commented Feb 8, 2021


		# Ensure the instance pointed to by the weak reference is no longer available.
		self.assertIsNone(target_weak())

	return '%s(%r, %r, tranform=%r)' % (
	return '%s(%r, %r, transform=%r)' % (

	- The first lookups are performed on the unique constraintss sharing the most parameters
	- The first lookups are performed on the unique constraints sharing the most parameters

		@@ -1048,4 +1082,3 @@ class Meta:

		# Ensure the instance pointed to by the weak reference is no longer available.
		self.assertIsNone(target_weak())

	self.assertEqual(dict(a='foo_0', b=2, c=3, d=4), TestModel.objects.last_lookup)
	self.assertEqual({"a": 'foo_0', "b": 2, "c": 3, "d": 4}), TestModel.objects.last_lookup)

		@@ -2293,15 +2299,17 @@ def test_full_get_or_create(self):
		class TestModelFactory(factory.django.DjangoModelFactory):

Add unique_constraints, a more versatile replacement for django_get_or_create and sqlalchemy_get_or_create #794

Are you sure you want to change the base?

Add unique_constraints, a more versatile replacement for django_get_or_create and sqlalchemy_get_or_create #794

Conversation

rbarrois commented Oct 11, 2020

rbarrois commented Oct 11, 2020

francoisfreitag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalvrz commented Feb 8, 2021