Skip to content
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

Interplay of converter and validator #1171

Closed
AdrianSosic opened this issue Aug 2, 2023 · 3 comments
Closed

Interplay of converter and validator #1171

AdrianSosic opened this issue Aug 2, 2023 · 3 comments

Comments

@AdrianSosic
Copy link

Hi there,

I have a general question about the intended interplay between converters, validators, and pre/post-init: What is the attrs-y way of defining an attribute that requires some validation before conversion?

To explain what I mean: I guess many (or all?) converters require an appropriate input to even run without errors in the first place. How can that input be validated if the clear order of execution in attrs is "validators after converters" (see "Order of Execution")? Of course, there are several "workarounds" to achieve this, but I wonder what is the actual intended way.

Here is an example inspired by an actual use case of mine. Suppose I want to declare an attribute that holds a normalized sequence of numbers. These are the options I see, none of which seems perfectly right for different reasons:

Option 1

Skipping validation altogether. The converter will simply fail for inappropriate input.

from typing import List, Sequence
from attrs import define, field

def convert_weights_plain(weights: Sequence[float]) -> List[float]:
    total = sum(weights)
    return [w / total for w in weights]

@define
class A:
    weights: List[float] = field(converter=convert_weights_plain)

A(["abc", 2., 3.])

Pros:

  • Straightforward, no special handling required.

Cons:

  • We have no control over the thrown exception surfaced to the caller. Rather, it depends on the specific conversion logic at hand. In this particular case, it just happens to be:
    TypeError: unsupported operand type(s) for +: 'int' and 'str'

Option 2

Effectively letting the converter take the role of a validator.

from typing import List, Sequence
from attrs import define, field

def convert_weights_validated(weights: Sequence[float]) -> List[float]:
    try:
        total = sum(weights)
        return [w / total for w in weights]
    except Exception:
        raise ValueError("Weights must be a sequence of floats.")

@define
class B:
    weights: List[float] = field(converter=convert_weights_validated)

B(["abc", 2., 3.])

Pros:

  • Custom exception handling with only minimal code changes.

Cons:

  • While this works in principle, it seems inappropriate because the responsibilities are shifted. Also, quote from the attrs docs: "Arguably, you can abuse converters as one-argument validators" --> The emphasis is on abuse, so it sounds like attrs is not selling this as the intended approach.

Option 3

Artificially moving the conversion after validation by placing it into __attrs_post_init__, in order to break with the fixed attrs order of execution.

from typing import List, Sequence
from attrs import define, field
from attrs.validators import deep_iterable, instance_of

def convert_weights_plain(weights: Sequence[float]) -> List[float]:
    total = sum(weights)
    return [w / total for w in weights]

@define
class C:
    weights: List[float] = field(
        validator=deep_iterable(
            member_validator=instance_of(float),
            iterable_validator=instance_of(list),
        )
    )

    def __attrs_post_init__(self):
        self.weights = convert_weights_plain(self.weights)

C(["a", 2.0, 3.0])

Pros:

  • Validation happens were it is supposed to be.

Cons:

  • Rather inelegant.
  • The converter field-argument is unused.
  • There is no more float conversion before the validator is executed. This means that the method only works for actual inputs of type List[float] and something like [1, 2, 3] would throw a validation error, which is not desired. Accordingly, the instance_of check in the validator would need to be adjusted as well.

Option 4

Similarly to Option 3, one could artificially enforce a "validation-before-conversion" execution order by placing the validation step into pre-init. However, this approach comes with the same drawbacks, so I'll skip it.

Option 5

Outsourcing the validation to cattrs.

Pros:

  • Minimal changes compared to Option 1.
  • We get an actual validation error (though no custom one).

Cons:

  • Still, the validation is actually performed inside the converter and not in the validator, which seems like a shift in responsibilities.
from typing import List, Sequence
from attrs import define, field
from cattrs import structure

def convert_weights_plain(weights: Sequence[float]) -> List[float]:
    total = sum(weights)
    return [w / total for w in weights]

@define
class D:
    weights: List[float] = field(
        converter=[
            lambda x: structure(x, List[float]),
            convert_weights_plain,
        ]
    )

print(D(["a", 2.0, 3.0]))

The attrs way

This brings me again to the question: what is the intended use of the provided mechanisms? Of course, the same question applies to simpler settings (e.g. converter=int, which would also just fail if a string is passed as the argument) but I thought an example that goes beyond pure type conversion would be useful to make the point.

If there is no more elegant way, I will go with Option 2, but I'd be happy to hear your opinions.

@hynek
Copy link
Member

hynek commented Aug 15, 2023

This is a question as old as attrs and we've spent much time trying to come up with a perfect solution, but came up empty.

I think the best answer is: for simple cases option 2, for complex cases option 5.

I suspect the perfect solution would be a nicer/more composable API for option 2.

@AdrianSosic
Copy link
Author

I see. But thanks for the answer anyway! I think this is also quite related to the discussion about three-argument converters (#709). And perhaps to share my opinion: I've also had several use cases where they would have helped and I found myself skimming the attrs documentation over and over again to see if this is somehow possible =)

@hynek
Copy link
Member

hynek commented Aug 24, 2023

It's been on top of my attrs todo list for months. 😒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants