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

Non-nullable columns with a default value should be optional #47

Open
smsearcy opened this issue Mar 18, 2024 · 4 comments · May be fixed by #48
Open

Non-nullable columns with a default value should be optional #47

smsearcy opened this issue Mar 18, 2024 · 4 comments · May be fixed by #48

Comments

@smsearcy
Copy link

smsearcy commented Mar 18, 2024

While switching to wtforms-sqlalchemy from another library I ran into an issue that we have columns with nullable=False and default value, which should be able to post, but the generated form had added a required validator to those fields so validation failed.

Example of columns:

description = Column(Unicode(20), default="", nullable=False)
active = Column(Boolean, default=True, nullable=False)

Validation result from submitting form:
image

The expected result is that those fields should not have been required so that form processing would continue. Do you see any issues with setting columns with a default to Optional?

Edit: It seems that a checkbox should never be required, right?

@mfisher87
Copy link
Contributor

mfisher87 commented Mar 21, 2024

This problem has been on the backburner for me for a little bit. Picked it back up today, and this hackaround seems to work:

class CustomModelConverter(ModelConverter):                                                                                                                                                    

    @converts("Boolean")
    def conv_Boolean(self, field_args, **_):
        """Hack: Prevent a required checkbox from failing client-side validation when False.

        "Unchecked" is a valid value: False!
        """
        field_args["validators"] = []
        return fields.BooleanField(**field_args)

My database field is not nullable, and it updates to false when I submit an unchecked field generated from my model with this converter. I have a deadline today but I'd like to take a stab at upstreaming this.

It seems that a checkbox should never be required, right?

I'm with you! Not as a front-end for a boolean database field at least, because it only lets you select one value. A required boolean field needs a way to submit false. If we display the checkbox as the input mechanism for this, unchecked must be false. A nullable boolean field, though, can have three values. A checkbox can't represent that. What happens in practice is unchecked means False and there's no way to submit NULL with this input mechanism. Not sure what the correct path forward for this library is in light of this. Should a required boolean be a checkbox, and a nullable boolean be a 3-choice radio button? 🤷

With a short discussion on the above I think I'd be comfortable trying to implement the decision.

My best stab at a concise problem summary: For a BooleanField generated with model_form(), nullable=True means the database field has 3 options, but the input field has 2 options (False/True). nullable=False means the database field has 2 options, but the input field has 1 (True, because client validation prevents False)!

@smsearcy
Copy link
Author

smsearcy commented May 10, 2024

@mfisher87, I think your comments about the checkbox summarize it well. I've been moving toward making fewer fields nullable (especially booleans), but that doesn't cover everyone's use case. And while it is true that a nullable boolean has three fields, how many people actually think of it as such? (Especially given Python's truthiness.) Representing boolean columns as something other than a checkbox could cause even more issues...

I took a stab at fixing this via the existence of a default specified for the column, which covers my current use cases but I'm not sure all the possible side effects. It might not cover the situation of a boolean column with default=True, but I need to look into how WTForms handles loading that data (that might be a problem even if the column is nullable).

Edit: One issue with the radio button, while it can start unchecked, once an option is checked, is there a way to uncheck it entirely? From the perspective of a form submission, if the field was rendered, there is no discernable difference between NULL and False at that point, so I think there is a certain logic to always passing False. But that's probably getting more into WTForms implementation, which I need to dig into a little more.

@mfisher87
Copy link
Contributor

mfisher87 commented May 10, 2024

One issue with the radio button, while it can start unchecked, once an option is checked, is there a way to uncheck it entirely? From the perspective of a form submission, if the field was rendered, there is no discernable difference between NULL and False at that point, so I think there is a certain logic to always passing False. But that's probably getting more into WTForms implementation, which I need to dig into a little more.

Yeah, there is no way to unselect a radio button without javascript that I know of. In my radio button suggestion I think I was imagining that there would be an explicit selection for NULL, e.g. labeled "N/A" or something. It's been a while since then so I'm not sure :)

@smsearcy
Copy link
Author

I confirmed that the solution in #48 works correctly (it will save the unchecked/false state) when default=True.

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

Successfully merging a pull request may close this issue.

2 participants