firecmsco / firecms

Awesome Firebase/Firestore-based CMS. The missing admin panel for your Firebase project!
https://firecms.co
Other
1.15k stars 184 forks source link

[regression bug] Optional fields left empty are populated with empty strings instead of `null` #512

Closed SimpleCreations closed 3 months ago

SimpleCreations commented 1 year ago

Bug description

Marked this as a regression because the issue was not present in 2.0.0-beta.4

An optional string field that was left blank is saved as "" to Firestore, even though the field config has min: 1 (explicitly marking that it cannot be an empty string).

Example field config:

      title: {
        name: "Title",
        dataType: "string",
        validation: { min: 1, trim: true },
        clearable: true,
      }

Expected behavior

If an optional field has min: 1 (or greater) and is left blank on save, the document in Firestore is expected to have null under this field. No red validation errors should be seen after save.

Actual behavior

The fields are highlighted red because they fail the min: 1 validation intended to prevent empty strings.

Screenshot 2023-06-23 at 13 07 43 Screenshot 2023-06-23 at 13 08 27

Workaround

The workaround is to press the "X" button to "clear" the field before saving, which sets its value to null instead of an empty string. This is of course unintuitive.

fgatti675 commented 1 year ago

Thank you, will check this

fgatti675 commented 1 year ago

Fixed in version 2.0.5 :)

SimpleCreations commented 1 year ago

Hi @fgatti675, thank you for all the work you put into this!

Unfortunately this is still an issue. To reproduce it (with the exact same setup) you'd need to enter something in the optional text field and then erase it. After saving the entity, you'll get the same red text box as I reported.

As I understand, you've made the default value null, but if the user submits an empty string after editing the field, it's currently not converted back to null (especially if min is greater than 0 which causes the red box).

SimpleCreations commented 1 year ago

Moreover, the workaround with clicking the "X" button to reset the value to null doesn't work anymore — I'm stuck with an empty string and a red box.

fgatti675 commented 1 year ago

To be fair, I have tested this setting the required flag to true. I understand that if the minimum length is 1, it means it is required. I think setting that flag should solve your issues? Or I am failing to understand your use case

SimpleCreations commented 1 year ago

The way I see it, required indicates whether the field is nullable or not, that is whether the value can be null/undefined. Even though many people use "" for a missing value instead of null/undefined, it is often incorrect. (For example, if is a variable is intended to have a web URL, it's incorrect to assign "" to it, because the string must start with http; instead, undefined or null should be used to indicate a missing value.)

min, on the other hand, is a property to validate contents of an existing value, specifically the length of the string.

Thus there's a distinction between a required field and a field with a minimum length of of 1. A required field is a field that cannot be null, and a min: 1 field is a field such as if its value is a string, its length cannot be 0.

The bug in FireCMS is that it allows the user to save an invalid state to Firestore. As a developer I want to declare a "nullable, but never empty string" field, and to achieve this I specified { required: false, min: 1 } to tell FireCMS that if the user entered nothing, I want the value to be null, otherwise I want the value to be whatever they entered. Basically allowing a missing value, but forbidding empty strings.

But this gives us a problem. Even though our data can be nullable, the HTML text box doesn't have a "null" state. So we must use as empty string for the initial value of the text box... And ways to implement this may vary. Easiest way would be to convert "" to null if min is above 0.

To summarize, my use case is — I want my Firestore to have null if the field was left empty, and to have a non-empty string if the user entered something.

I hope I made this more clear! Please let me know if there's anything I can help with.

EDIT: I think the best solution would be to always convert "" to null if the field is required: false. Checking min wouldn't solve the issue because I can declare a regex that'd ban empty strings, and then we'd have the same bug again.

fgatti675 commented 3 months ago

Guys I am closing this ticket for lack of activity. There have been no complaints since. If it is still an issue, feel free to reopen