Fixed #25370 - Helpful exception message for lamda #13582

craigiansmith · 2020-10-21T05:12:32Z

Lambda expressions can't be serialized. Make the exception message more
helpful when a lamda is used as an argument to a model field.

Lambda expressions can't be serialized. Make the exception message more helpful when it is attempted.

felixxm

@craigiansmith Thanks for this patch 👍

felixxm · 2020-10-21T06:44:50Z

django/db/migrations/serializer.py

+            msg = 'During serialization of the field'
+            e.args = ('{} {} the following blew up: {}'.format(msg, self.value, e.args[0]), ) + e.args[1:]
+            raise e


I don't see a reason for modifying a source exception, you can use:

raise ValueError(...) from e

Also blew up is not a appropriate wording and a new exception is not more informative because it refers to the field class ... the field <django.db.models.fields.CharField> instead of <app_label>.<model_name>.<field_name>, maybe:

raise ValueError('Error during %s serializing: %s' % (field, e)) from e

I don't have a quick answer how to get a field path.

@felixxm thanks for the feedback. I have changed the error message as you suggested. But I have kept the .format notation instead of %s, if that should also change, please let me know. I will look into how to get the field path before I update with a new commit.

@felixxm self.value shows the field path in the <app label>.<model name>.<field name> form. I have tested this by starting a django project using the amended source code and attempting to makemigrations for an app with a model that has a field that uses a lambda expression.

I have had trouble getting the test to verify this behavior though, since when I try to import a model from migrations.models and serialize it, the field is treated as a DeferredAttribute instead of a model field and so doesn't get handled by the ModelFieldSerializer. When I tested in a new project I was able to get the desired behavior by changing a field on a model that already had migrations, so the only thing that had to be serialized was the field which I amended to use a lambda expression as its default. At this stage, I don't see how to achieve that in a test. Any suggestions?

By making a custom field the test error message shows the desired field path.

No it's not, without this patch it raises, e.g.

ValueError: Cannot serialize function: lambda

with this patch:

ValueError: Error during <django.db.models.fields.IntegerField> serializing: Cannot serialize function: lambda

It's not more descriptive, IMO. You can have hundreds of IntegerFields in dozens of apps. I would expect:

ValueError: Error during 'test_app.models.MyModel.field_1.default' serializing: Cannot serialize function: lambda

I don't see much value in this change if it's not feasible to get <app label>.<model name>.<field name>.<parameter> or at least <app label>.<model name>.<field name>.

Maybe we should fix this in FunctionTypeSerializer, it should be doable to get at least test_app.models.MyModel from __qualname__ and __module__ 🕵️

The FunctionTypeSerializer has the same issue, if the field isn't part of a model.

I need to figure out how to add the UnserializableField to a migrated model, within a test. So that the field is a member of a model, and that the model appears as though it is already migrated, and that the only change is to add the UnserializableField. Any ideas on how to do this?

@felixxm the latest commit seems to have gotten the desired result.

I'm sorry but it's still the same 🤷 . You can manipulate with tests to create an unreachable state where you will get an expected message but that's not the correct solution.

Please check an attached project and try to run:

$> python manage.py makemigrations

it raises:

ValueError: Error during <django.db.models.fields.IntegerField> serializing: Cannot serialize function: lambda

Hi @felixxm, you were right, the error message can be constructed in the FunctionTypeSerializer - but I had trouble getting the field name, any ideas on that one? BTW thanks for the welcome aboard message

Thanks for updates 👍

... but I had trouble getting the field name, any ideas on that one?

Unfortunately not, moreover I'm afraid that we will not be able to get <app label>.<model name>.<field name> or even <app label>.<model name> in a reliable way. We serialize field from django.db.models not a model attribute, that's why it's complicated or even not feasible. Each approach doesn't work in some cases, e.g. constructing messages in the FunctionTypeSerializer will not work for lambdas defined in the module:

Error during serializing test_one.models.<lambda>: ...

or imported from other modules:

ValueError: Error during serializing test_one.utils.<lambda>: ...

I think we should close this as wontfix 😞

- Changed error message for lambda serialization - Confirmed that error message shows field module path

jacobtylerwalls · 2020-10-22T03:02:17Z

tests/migrations/test_writer.py

@@ -252,9 +252,10 @@ def test_serialize_constants(self):
        self.assertSerializedEqual(False)

    def test_trying_to_serialize_a_lambda_in_field_argument_should_raise_helpful_message(self):


A test name over 79 characters seems too verbose to me.

Maybe: test_serialize_field_with_lambda ?

Thanks for the suggestion. I will update it.

- Change test name to make it more succinct - Use a custom field class to demonstrate correct field path in test

smithdc1 · 2020-10-22T04:35:36Z

django/db/migrations/serializer.py

+        try:
+            return self.serialize_deconstructed(path, args, kwargs)
+        except ValueError as e:
+            raise ValueError('Error during {} serializing: {}'.format(self.value, e)) from e


Guidance is to try and avoid format. Do you think this could be more readable written as an f string or % formatting?

Thanks, you're right. It's better to use an f string.

smithdc1 · 2020-10-22T04:43:44Z

tests/migrations/test_writer.py

+    def test_serialize_field_with_lambda(self):
+        field = UnserializableField()
+
+        error = 'Cannot serialize function: lambda'


Suggestion, write this as one string in full?

msg = ( "Error during...." " ....." )

By using an f string it was easy to fit it all on one line without exceeding the character limit.

- Use f string instead of string.format - Use a single string for test message

Test shows that the error message states app_label.model.field

Construct error message in FunctionTypeSerializer instead of ModelFieldSerializer. Remove migration for the associated test. Rename the model used to test this.

felixxm · 2020-11-12T14:56:59Z

@craigiansmith Thanks for your efforts! 🥇 Unfortunately, IMO it's not doable.

Closing per ticket.

Fixed #25370 -- Helpful exception message for lamda

3623f6c

Lambda expressions can't be serialized. Make the exception message more helpful when it is attempted.

craigiansmith force-pushed the ticket_25370 branch from 7ec32b1 to 3623f6c Compare October 21, 2020 05:38

felixxm reviewed Oct 21, 2020

View reviewed changes

Made changes asked in review by felixxm

96f993a

- Changed error message for lambda serialization - Confirmed that error message shows field module path

jacobtylerwalls reviewed Oct 22, 2020

View reviewed changes

Made change asked in review by jacobtylerwalls

d423cac

- Change test name to make it more succinct - Use a custom field class to demonstrate correct field path in test

smithdc1 reviewed Oct 22, 2020

View reviewed changes

craigiansmith added 3 commits October 23, 2020 09:58

Made changes asked in review by smithdc1

6fc6922

- Use f string instead of string.format - Use a single string for test message

Made changes asked in review by felixxm

a2e8aa3

Test shows that the error message states app_label.model.field

Made changes asked in review by felix

9f2990d

Construct error message in FunctionTypeSerializer instead of ModelFieldSerializer. Remove migration for the associated test. Rename the model used to test this.

felixxm closed this Nov 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #25370 - Helpful exception message for lamda #13582

Fixed #25370 - Helpful exception message for lamda #13582

craigiansmith commented Oct 21, 2020

felixxm left a comment

felixxm Oct 21, 2020

craigiansmith Oct 21, 2020

craigiansmith Oct 22, 2020 •

edited

craigiansmith Oct 22, 2020

felixxm Oct 22, 2020

craigiansmith Oct 29, 2020

craigiansmith Oct 29, 2020

felixxm Nov 3, 2020

craigiansmith Nov 12, 2020

felixxm Nov 12, 2020

jacobtylerwalls Oct 22, 2020

craigiansmith Oct 22, 2020

smithdc1 Oct 22, 2020

craigiansmith Oct 22, 2020

smithdc1 Oct 22, 2020

craigiansmith Oct 22, 2020

felixxm commented Nov 12, 2020

		@@ -252,9 +252,10 @@ def test_serialize_constants(self):
		self.assertSerializedEqual(False)

		def test_trying_to_serialize_a_lambda_in_field_argument_should_raise_helpful_message(self):

Fixed #25370 - Helpful exception message for lamda #13582

Fixed #25370 - Helpful exception message for lamda #13582

Conversation

craigiansmith commented Oct 21, 2020

felixxm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigiansmith Oct 22, 2020 • edited

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

felixxm commented Nov 12, 2020

craigiansmith Oct 22, 2020 •

edited