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

Toggle interface doesn't work properly with MySQL's tinyint(1) #8480

Closed
3 tasks done
RemiMatrod opened this issue Oct 1, 2021 · 16 comments
Closed
3 tasks done

Toggle interface doesn't work properly with MySQL's tinyint(1) #8480

RemiMatrod opened this issue Oct 1, 2021 · 16 comments

Comments

@RemiMatrod
Copy link

Preflight Checklist

Describe the Bug

The Toggle interface doesn't work properly when used on a tinyint(1). It will always display the value as disabled (false) even when the tinyint's value is 1.
Clicking on it once seems to make it update properly (click once and the value stays at disabled, but the edit page says it can save and update the changes, click once more and the value is set as enabled). But the interface will always be incorrect when opening the edit item page if the tinyint(1) value is 1 (true).

To Reproduce

  • Create a new table in MySQL, with a tinyint(1) column
    table create
  • Configure the new table in Directus
  • Add the Toggle interface and the Boolean display to the tinyint(1) column
    toggle config
    display config
  • Create a new row with the value set to 1 (enabled)
    creating

Now the display will show that the value is correctly set to true, but when editing the row the Toggle interface will say the value is not enabled. However, opening it as a raw value shows that the value is correctly set to 1.
display show
toggle show
raw value

What version of Directus are you using?

v9.0.0-rc.95

What version of Node.js are you using?

16.4.0

What database are you using?

MySQL 5.7.34

What browser are you using?

Chrome

What operating system are you using?

Windows

How are you deploying Directus?

Running locally

@rijkvanzanten
Copy link
Member

Ref #6665, #7287

@RemiMatrod
Copy link
Author

Hello,
I've been keeping track of the updates on the PRs #8482 and #8497, and saw the latter one was merged and put in the release rc.101.
To give some updates about my setup, after updating my Directus version to rc.101 and testing the issue I explained here, it looks like the merged fix in #8497 didn't change anything for me (looks like @paulboudewijn was right, as they stated here #8482 (comment) that their PR wouldn't solve my issue).

@paulboudewijn
Copy link
Contributor

Hi @RemiMatrod ,

Could you try what happens if you make the tinyint column not nullable ?

@RemiMatrod
Copy link
Author

My tinyint column is already set at NOT NULL, here's my current table creation statement:

CREATE TABLE `doc_test` (
   `id` int(11) NOT NULL,
   `is_set` tinyint(1) NOT NULL,
   PRIMARY KEY (`id`)
 ) ENGINE=InnoDB DEFAULT CHARSET=latin1

And here, we can see that under these conditions, the checkbox is shown as disabled even though the raw value is set at 1:
image

To make sure that this happens again, I decided to try recreating a toggle column on the newest version of Directus, by forcing the required attribute to be enabled:
image
And in that case, the problem isn't there on the new value, but the old value still has the same issue, even when it's set in Directus to be a required attribute.
image
image

Here is the updated table contents:

CREATE TABLE `doc_test` (
   `id` int(11) NOT NULL,
   `is_set` tinyint(1) NOT NULL,
   `is_set_new` tinyint(1) DEFAULT NULL,
   PRIMARY KEY (`id`)
 ) ENGINE=InnoDB DEFAULT CHARSET=latin1

And here is the content of my directus_fields for this table:
image

If there is a way to make the older attributes work the same way as the new fixed ones, it would be great 😀

@RemiMatrod
Copy link
Author

I think I found something:
If I update the special attribute of my tinyint(1) column in directus_fields to be the value boolean, and then activate the Required attribute in the fields configuration of my column, it looks like it fixes the display problem.
I still have to edit the database manually to enable this special attribute, but after this the displayed boolean value is correct 😃

@rijkvanzanten
Copy link
Member

Ah that makes total sense actually, and works as expected. That special flag instructs the API to treat the column as a boolean. When you setup the field from Directus, it'll set that flag, which is why I wasn't able to repro it before (as I created the field in Directus, and then change the type afterwards).

The main question becomes how and if to fix that 🤔 An unsigned tinyint(1) is technically any number from 0-255, so we wouldn't want to require that to be a boolean.

I think the only way forward here is to treat this as a new feature, where the app will ask you what type to use for an existing column once you select it in the data model settings. In this case, it's ambiguous whether to use an integer (as per the type "strictly") or a boolean (which is arguably a more common use for tinyint(1)). Tricky stuff!

@paulboudewijn
Copy link
Contributor

I would think a column of type tinyint(1) should be treated as being a tiny int, because that's what it is.
If one decided to use the checkbox interface on this column, this should simply just limit the possible values to 0 and 1.

I don't see the point of using a special flag, as we know the column datatype and interface type, that should be enough to map the value from interface to column datatype and vice versa.
Or am I missing something?

@rijkvanzanten
Copy link
Member

Or am I missing something?

Yeah, it's the API that does the data casting based on the value 👍🏻 Instead of having the app convert 1 to true all over the place, the API handles this to standardize the output for columns that are intended to be used as a boolean. (Similarly, CSV values use JSON Arrays for IO, Dates are done in ISO8601, etc)

@paulboudewijn
Copy link
Contributor

That's exactly how I would think it should work. But since the API knows which interface is used and what the underlying datatype is, casting the value from tinyint to bool and vv shouldn't be a problem for the API, right?

@rijkvanzanten
Copy link
Member

Right, but we can't use it exclusively on the interface, as interfaces can be user-created, so we can't keep a list of the type based on the interface. Also, you might not even have an interface configured in the first place! Right now, having a dedicated flag that says treat this thing as X turned out to work more reliable for this use case, though we have to figure out a way to set it from the app when you're integrating with existing columns 👍🏻

@paulboudewijn
Copy link
Contributor

, but we can't use it exclusively on the interface, as interfaces can be user-created

Agree, I didn't think of user-created interfaces.

Also, you might not even have an interface configured in the first place!

If you don't have an interface configured, the column should just be treated as being a tinyint, with a normal input field to enter an integer value.

@paulboudewijn
Copy link
Contributor

Is the edgecase on line 130 in api/src/utils/get-local-type.ts still necessary now there's the special flag?
In my opinion this type of edge case programming makes code very messy.

@paulboudewijn
Copy link
Contributor

I think the only way forward here is to treat this as a new feature, where the app will ask you what type to use for an existing column once you select it in the data model settings.

How about adding some sort of hidden options to the interface configuration which we can use to set the 'special flag'.

So a tinyint column is beeing mirrored bij the API as being a tiny int, because that's what it is.
But when an admin decides this tinyint column should use the boolean interface (or a user-created interface with the same hidden special flag option), the special flag is set and the API treats it as being a boolean.

No edgecase configuration options in the app are needed, adding a existing column just works the same as adding a new one through directus. Edge case programming in the API is limited to processing the special flag.

@rijkvanzanten
Copy link
Member

I believe this was resolved in #10083. We can reopen if the problem persists in 9.1.3+

@jacqueslareau
Copy link

jacqueslareau commented Mar 30, 2022

I'm having this problem on externally created tables. Tried setting the special field to cast-boolean and still doesn't work.

Edit: Ok so after setting the special field to cast-boolean, Directus needs to be restarted.

@rasco
Copy link
Sponsor

rasco commented Mar 31, 2022

I'm also having this problem on externally created tables.
MySQL, tinyint(1)
I altered the column and made it a regular boolean. Still doesn't work.
Even after i set the special field to boolean

Ahhh, I see, the special field has to be cast-boolean, and then Directus restart -> that worked

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants