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

Value is not a valid <type> when having column with null=True, default=None, required=False #290

Open
metakot opened this issue Mar 2, 2023 · 11 comments

Comments

@metakot
Copy link

metakot commented Mar 2, 2023

piccolo==0.107.0
piccolo-admin==0.44.0
piccolo-api==0.51.0

So I have this simple table

import piccolo.columns as cl
from piccolo.table import Table

class TestTable(Table):
    number = cl.Integer(
        null=True,
        default=None,
        required=False,
    )

When I try to add the record in admin, it yells on me
image

Happens also with other fields like UUID.
What can we do here?

@sinisaos
Copy link
Member

sinisaos commented Mar 2, 2023

@metakot This error occurs because None is not a valid int type, and the int type is used to store whole numbers. If you remove default=None or set default=some valid integer, everything works. The same goes for UUID as None is not a valid type for UUID column default value. Hope that helps.

@dantownsend
Copy link
Member

I just looked, and it's sending an string to the backend instead of null:

Screenshot 2023-03-02 at 18 30 04

Possible fixes - set the default to 0 for now.

In terms of a proper fix, we could automatically convert an empty string value to null for integer columns. If we then gave it a placeholder of NULL, it would work quite nicely.

Screenshot 2023-03-02 at 18 34 13

@metakot
Copy link
Author

metakot commented Mar 2, 2023

@metakot This error occurs because None is not a valid int type, and the int type is used to store whole numbers. If you remove default=None or set default=some valid integer, everything works. The same goes for UUID as None is not a valid type for UUID column default value. Hope that helps.

Sorry, but no. This is nonsense. I can remember a lot of cases there database table has NULL fields and this is ok. The point here was what the piccolo admin cannot work with NULL fields at all (except for FK fields). Ok, I've checked and it works with datetime fields also. So you insist what we can use nullable datetime fields, but not ints?

Also in django it works =)

@metakot
Copy link
Author

metakot commented Mar 2, 2023

@dantownsend

convert an empty string value to null for integer columns

Not only integers, but ALL nullable fields except the varchar. And what to do with nullable varchar -- this is different story. Because empty string and NULL is different values with different meaning.

@metakot
Copy link
Author

metakot commented Mar 2, 2023

As for nullable datetimes:
image
This works!

But if I fill the datetime and then changed my mind, I cannot force the null here, there is no button:
image

@metakot
Copy link
Author

metakot commented Mar 2, 2023

Both Client and Car FK fields has null=False, and guess what? The admin shows the None in the available options.
image

Then totally expected database error emerges about violating not-null constraint.
required=True does not help here, it just renders the VERY subtle * at the field name, but the None option is still visible.

@dantownsend
Copy link
Member

Yeah, null is an option for some fields but not others.

You're right about Varchar - we'll need a 'null' checkbox for that to distinguish between empty strings.

We need to check all of the nullable fields to see which ones work.

@sinisaos
Copy link
Member

sinisaos commented Mar 2, 2023

Sorry, but no. This is nonsense. I can remember a lot of cases there database table has NULL fields and this is ok. The point here was what the piccolo admin cannot work with NULL fields at all (except for FK fields). Ok, I've checked and it works with datetime fields also. So you insist what we can use nullable datetime fields, but not ints?

@metakot I'm sorry I offended you so much. I just wanted to help and I was wrong. Cheers.

@dantownsend
Copy link
Member

@sinisaos Nothing to apologise for - you were helping as best you could 👍

I'll have a look into it. I think integer fields is probably a good place to start.

I don't tend to use many nullable columns in my projects besides foreign keys, but I know it'll bite me soon if any of my projects require it.

@dantownsend
Copy link
Member

I've added support for nullable number fields. I can't release it though because GitHub Actions is down.

For fields like varchar, text, uuid - try typing in null. That should work for now, but needs a better solution.

@dantownsend
Copy link
Member

dantownsend commented Mar 3, 2023

I released it manually on my local machine - try piccolo_admin==0.45.0. @metakot Give it a go, and let me know what you think.

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

3 participants