spiral-project / ihatemoney

A simple shared budget manager web application
https://ihatemoney.org
Other
1.18k stars 267 forks source link

fix: 'Bill Type: Invalid Choice: could not coerce' error #1294

Closed rriski closed 6 months ago

rriski commented 6 months ago

Error introduced in #1290. Fixes #1293. WTForms needs to be bumped to >=2.3.2 as it includes a fix to SelectField which is required for this change to work.

See:

TomRoussel commented 6 months ago

I've been playing around with the changes in this PR. To me it seems like BillType.coerce in unnecessary. We can just use the enum constructor for this. BillType("Expense") correctly returns BillType.Expense.

I don't really understand why, but I think the key change that fixes the error in the form is adding BillType.__str__?

TomRoussel commented 6 months ago

I quickly pushed my change to explain what I mean: See this commit.

When I run the server with this change, I also don't get the error message.

TomRoussel commented 6 months ago

Okay, I figured out what's going on. The WTForms SelectField sends the different choices to the browser as strings (documented here. These are specified by BillType.choices() as a list of (value, label) tuples. The value should be the actual enum instance, and label would then be what the user sees. Since WTForms encodes everything as a string, it also sends this value to the browser as str(value). For an ordinary enum, this becomes "BillType.EXPENSE" for example. When the browser then sends the post request, it uses that string, but that's not understood by the coerce function.

That's why adding an explicit BillType.__str__ fixes the problem. It's actually the equivalent as modifying choices as follows:

    @classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]
rriski commented 6 months ago

Okay, I figured out what's going on. The WTForms SelectField sends the different choices to the browser as strings (documented here. These are specified by BillType.choices() as a list of (value, label) tuples. The value should be the actual enum instance, and label would then be what the user sees. Since WTForms encodes everything as a string, it also sends this value to the browser as str(value). For an ordinary enum, this becomes "BillType.EXPENSE" for example. When the browser then sends the post request, it uses that string, but that's not understood by the coerce function.

That's why adding an explicit BillType.__str__ fixes the problem. It's actually the equivalent as modifying choices as follows:

    @classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]

Thanks, I have updated the PR to use:

@classmethod
    def choices(cls):
        return [(choice.value, choice.value) for choice in cls]
TomRoussel commented 6 months ago

That looks good to me! Ah nice, you also caught the duplicate tests, I just saw those today.

zorun commented 6 months ago

Thanks for the fix.

Our CI was failing, which means that unformatted code went through in the last PR. I have fixed the issue in #1296 and hopefully reorganized the CI to avoid this in the future.

It does mean that you have to rebase and possibly fix conflicts, sorry about that. But we should then get a clearer view of the CI.

zorun commented 6 months ago

The testcases with the lower versions for dependency are failing with HTTP 400 errors. It means that the approach doesn't work with older version of some libraries (here, most probably wtforms)

Does your solution rely on a undocumented behaviour in wtforms? If so, it's preferable to find another solution that will be less fragile over time.

If there is really no other workable approach, you can try bumping the minimum version of wtforms.

TomRoussel commented 6 months ago

Very strange, as far as I can tell, this solution is not using anything exotic from wtforms. All api calls are following the documented way of working for 2.3.x of wtforms, which should correspond to the minimal version. What I think is strange is that this test is breaking now, but the minimal version checks in #1290 were fine.

I can try replicating the problem when I have time later this week.

TomRoussel commented 6 months ago

I found the culprit I think. I believe the problem is related to a bug fixed in a PR of WTForms here. The changelog for 2.3.2 mentions that they fixed a bug in the SelectField.choices field. Using WTForms 2.3.1 fails, but 2.3.2 succeeds. Reading the PR description, I'm really not sure why our implementation here doesn't work. The only reason I can think of is that their validation logic for 2.3.1 somehow breaks if the choices tuples are identical values?

Is see 2 solutions to this problem, either we do a minor version bump for WTForms from 2.3.1 --> 2.3.2, or we use the following workaround which also seems to do the trick:

class BillType(Enum):
    EXPENSE = "Expense"
    REIMBURSEMENT = "Reimbursement"

    def __str__(self):
        return self.value

    @classmethod
    def choices(cls):
        return [(choice, choice.value) for choice in cls]

Using a BillType instance as the first member of the choice tuple seems to not trigger the validation bug, while the str method makes sure flasks sends a sensible value to the browser which can then be coerced back to the enum.

By the way, sorry @rriski that you got involved in this weird can of worms from my implementation!

zorun commented 6 months ago

Thanks for looking at it!

If it's a bug in wtforms, then let's use the simplest solution: @rriski , can you bump the wtforms dependency to >=2.3.3 ?

rriski commented 6 months ago

Thanks for looking at it!

If it's a bug in wtforms, then let's use the simplest solution: @rriski , can you bump the wtforms dependency to >=2.3.3 ?

Sure, updated PR now. Thanks @TomRoussel for figuring out the issue.