Django-rest-framework: Relax redundant method name restriction for SerializerMethodField

[gtaylor]

As per the 3.0 release, we now get AssertionErrors if we pass in a method name that matches the default:

AssertionError: It is redundant to specify `get_id_salted` on SerializerMethodField 'id_salted' in serializer 'UserSerializer', because it is the same as the default method name. Remove the `method_name` argument.

I can certainly see an argument for this sort of thing, but I subjectively prefer to be as explicit as possible, even if it comes at the cost of a few extra keystrokes. At any time, our team has external contractors (who may or may not have worked with DRF before) coming in and out, so this is one of those little easy hints we can give them. That arg (even if it's not needed anymore) provides some additional context for someone who is getting their bearings.

Would you accept a PR with a setting to disable this assertion? It'd help with those of us who prefer to be very explicit, and would also improve the migration path for those who don't care to make this change for many hundreds of serializer fields in larger codebases.

[tomchristie]

Would you accept a PR with a setting to disable this assertion?

No I don't think we should have it as a setting. We could reconsider the check tho.

[gtaylor]

Is there anything I can do to help with that?

I'm sure there are others that enshrine "Explicit is better than implicit" as much as I do. I feel like Django is heading in the direction of being more explicit over time, too (starting with the days of magic-removal).

[tomchristie]

Is there anything I can do to help with that?

You may as well issue the PR removing it, check if there's any tests expecting the assertion error that now fail, and also in the same PR update/remove any bits of docs that mention it, eg the 3.0 announcement.

[gtaylor]

Done! See the PR and make sure you are OK with this assert being removed from the Field parent class. I don't think there's a great reason to pass a redundant source param to something like a CharField, but I'm not sure we'd want to police that and not SMField. Seems like that could be handled by the users of DRF during their code reviews. I suppose there's still a case like this where it could be funky looking:

some_field = CharField(source='doesnt_match')
# Looks kind of out of place?
matching_field = CharField()
another_field = CharField(source='doesnt_match_either')

[tomchristie]

Seems like that could be handled by the users of DRF during their code reviews.

No - I've seen it plenty of times in 2.x codebases, and it's harmful to the ecosystem as it spreads uncertainty about if it's required or not. We can consider SerializerMethodField, but I'd def be against this change for source.

[gtaylor]

How would you like to handle the exemption for SMField but not Field? We'd need to disable the check, and I imagine we'd want to avoid messing with the signature of bind(). Would a (private?) class variable be OK? Could override in sub-classes to give people easy control over this behavior in custom fields.

[tomchristie]

They're different checks. Don't affect each other. I'm still not totally convinced this change is something we want tho, enforced consistency is def a good thing.

[gtaylor]

Maybe I'm reading this wrong, but I think the assertion check would come up from the parent class (Field) here: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/fields.py#L1237

Wouldn't we need to suppress that?

[tomchristie]

On reflection I'm going to close this off. The behaviour as it currently stands is deliberate design and I'm happy with the benefit of enforcing a single consistent style.

[masterpi314]

I just ran into this on a case you're probably not considering: the source argument is coming from a lookup in a global dictionary that is used in other places. The dictionary currently contains mostly identity mappings, but my serializer should respect that dictionary should it ever change. I can't do that right now without jumping through major hoops because of this assertion, which is totally unnecessary to the functioning of this class. I don't honestly care if you remove it or not because I won't be able to use the new version before it matters, but IMO this is an ill-considered design decision.

[rpkilby]

Hi @masterpi314. It's difficult to appreciate your use case without seeing a more complete example. e.g., it's not clear if there's another way to workaround your issue. Regardless, it shouldn't be too difficult to override the bind method to exhibit the desired behavior.

Just make sure that you call super(SerializerMethodField, self).bind(field_name, parent), to skip SerializerMethodField's bind implementation.

[dms-cat]

The implicit relationship between a SerializerMethodField and a method means that the code is harder to read than specifying method_name=get_whatever.__name__. Specifying the method name like this also means refactoring tools can rename the method. My team is literally working around this by renaming getter methods.

Please reconsider disabling this assertion. Would you be willing to accept a similar PR for only this change?

Apropos: Have you considered just having a method argument rather than method_name?

[rpkilby]

I am +0 on relaxing this restriction. On the one hand, I understand that consistency is valuable - it would be undesirable if redundant method names are provided inconsistently. On the other hand, I also prefer explicitly providing the method name, even if it does happen to be redundant.

I'll leave this to Tom, but if this isn't accepted, a minimal implementation to require a method is:

class SerializerMethodField(serializers.Field):
    def __init__(self, method, **kwargs):
        self.method = method
        kwargs['source'] = '*'
        kwargs['read_only'] = True
        super().__init__(**kwargs)

    def to_representation(self, value):
        method = getattr(self.parent, self.method)
        return method(value)

[tomchristie]

Ran into this working with a client. Probably would be okay with us relaxing the source assertion.