Nested validation bug with version 3.6.0

[hip3r]

Describe the bug
object or list widgets, that are nested inside parent object widget don't pass validation anymore.
Object has all fields optional, but is detected as required.
Empty list is detected as invalid. Adding and removing one item solves issue and empty list is then detected as valid.

To Reproduce
config.yml settings for fields:

{name: usefulInfo, label: useful info, widget: object, i18n: true, collapsed: true, summary: '{{fields.title}}', fields: [
      {name: title, widget: string, i18n: true, required: false},
      {name: text, widget: markdown, minimal: true, required: false, editor_components: []},
      {name: button, widget: object, i18n: true, fields: [
        {name: text, widget: string, i18n: true, required: false},
        {name: href, widget: string, i18n: true, required: false},
      ]},
      {name: linksTitle, label: links title, widget: string, i18n: true, required: false},
      {name: links, widget: list, label_singular: link, i18n: true, fields: [{name: text, widget: string, i18n: true}, {name: href, widget: string, i18n: true}]},
    ]}

try saving entry with empty optional fields

Expected behavior

• object with optional fields should be valid if empty.
• empty list (of object that has required fields) should be valid

Screenshots

Applicable Versions:

• Decap CMS version: 3.6.0
• Browser version: Chrome

CMS configuration

backend:
  name: test-repo

publish_mode: editorial_workflow
media_folder: assets/uploads
local_backend: true

collections:
  - name: pages # a nested collection
    label: Pages
    label_singular: 'Page'
    folder: _pages
    create: true
    nested: { depth: 100 }
    fields: [
     {name: usefulInfo, label: useful info, widget: object, i18n: true, collapsed: true, summary: '{{fields.title}}', fields: [
      {name: title, widget: string, i18n: true, required: false},
      {name: text, widget: markdown, minimal: true, required: false, editor_components: []},
      {name: button, widget: object, i18n: true, fields: [
        {name: text, widget: string, i18n: true, required: false},
        {name: href, widget: string, i18n: true, required: false},
      ]},
      {name: linksTitle, label: links title, widget: string, i18n: true, required: false},
      {name: links, widget: list, label_singular: link, i18n: true, fields: [{name: text, widget: string, i18n: true}, {name: href, widget: string, i18n: true}]},
    ]}]

Additional context
could be caused by #7374

[hip3r]

@fgnass could you take a look?

[demshy]

Seems like the problem is here: https://github.com/decaporg/decap-cms/pull/7374/files#diff-994779730bc8166d032642e546bf8033d8ad53fe2cb05bb16d6f6e488271087dR82

Changing the validation here to

      if (control?.innerWrappedControl?.validate) {
        control.innerWrappedControl.validate();
      } else {
        control?.validate?.();
      }

fixes this for me, but at this point it is more guesswork then a thorough solution. I'd need more testing to be sure, but I can release a patch fairly quick. Then we can add an e2e test for this and find a better solution

[ryangittings]

+1 - this is causing many errors with my setup.

[fgnass]

I think I can explain why this is happening:

When required: false is not explicitly set on a list widget, it defaults to required: true. In validateSize() empty lists are only allowed when the list is not required.

For objects, a similar rule applies in validatePresence(). Even if all fields inside the object are optional, the object itself will be considered invalid if empty unless required: false is explicitly set on the object widget.

Looking at the code, I would assume that the behavior we now see is the one intended by the original authors, but it was previously hidden by a bug. Manually adding required: false to the config of the list and object widget configuration mitigates the problem and makes the form work as expected. To be honest, I have no idea how this should be handled in a way that does not break existing configs.

[ryangittings]

So for example - if we have an optional button in a object block - what is the correct syntax for that? Before I had to use required: false on both my link and text attributes within my button object to make the button object "not required".

[martinjagodic]

@fgnass, we agree that this has been technically a bug since the early days. But because it's been there for so long, it became a feature/part of how we set up our configs.

Because the impact is substantial for many projects, @demshy released a patch that restores the original behavior.

[ryangittings]

No problem - but my question is, what is the correct way to approach this with Decap 3.6.0?

[martinjagodic]

@ryangittings We reverted it in 3.6.1, so things should work as before 3.6.0. Besides testing if the patch works, there's nothing that you need to do.

[hip3r]

Docs say that required field is available on all fields. This needs update.

In case #7374 will still be worked on, note that list widget behaviour is bug in any case. Either it should constantly detect empty list as invalid, or constantly as valid.
Behaviour of Decap Cms 3.6.0 is that by default, empty list is detected as invalid. But adding and removing one item (resulting in empty list again) is detected as valid.

[fgnass]

First of all, I sincerely apologize for unintentionally introducing a breaking change. I think I was too confident due to the passing test suite – lesson learned.

Regarding the proposed patch: Wouldn’t this change mean that validation rules on list widgets (like min/max) would never be triggered? Please correct me if I'm wrong, since I haven't had a chance to test the fix myself yet.

I'm curious about the best way to address this issue moving forward. Here are a few potential approaches that come to mind:

1. Default to required: false for list and object widgets when not explicitly set: This would match the previous de facto behavior if I understand the situation correctly.

2. Fix List's validateSize() method: Set min to default to 0, and skip the upper bound validation when it's undefined.

3. Add a "Set/Remove" toggle for objects that are not required: This would help differentiate between an empty object and an undefined value. Without such a toggle, having a required flag for objects doesn't seem meaningful.

I’d love to hear your thoughts on these ideas, @martinjagodic and @demshy.

[demshy]

No worries, it is not the first time that we figure out how people actually use Decap despite the docs and tests suggesting otherwise :)
I think the important thing is, that we add some e2e tests for these cases after we figure out how to tackle them.

I tested the lists validations just now and they do seem to work with the patch, but I admit I did not dig deep enough to be able to explain why.

Defaulting required to false on these widgets does seem like the most efficient way now that I think about it. I do wonder why someone would ever turn this on though, unless we also added the set/remove option from point 3 (which is actually not a bad idea, right @martinjagodic ?).

[martinjagodic]

I agree with both of you.

Setting min: 0 as default will fix the issue with min being required if max is set (I can't find the issue where this is mentioned).

Set/remove will improve templating with Hugo because there are different types of empty objects right now.

Note: we have to update the docs when this is done. PR for the current state is already open: decaporg/decap-website#104

[hip3r]

In my opinion, object and list fields shouldn't have setting for required. Thinking it from UX, I would expect that object/list widget is valid if all it's children are valid. That's it. Adding extra setting to object/list widget can only confuse. Logically there's no need for it, or I missed something.

Let's take a look at list widget. Currently it accepts required setting, but works only if min and max settings are present. Logically required setting is obsolete, because setting min and max does the trick. Additionally it creates confusion if setting min and max and required: false, because it's the same as not setting anything.

I would propose that we remove required option for list widget (and leave object without as is).
Programatically these two widgets can default to required false, like proposed. (list widget still needs to validate size if min,max are set)