Attempt to add an affiliation for a newly added contributor results in web ui error

[yarikoptic]

index.e7a183c2.js:14 TypeError: this.value is not iterable
    at click (index.e7a183c2.js:191:39525)
    at ul (index.e7a183c2.js:10:23347)
    at o.n (index.e7a183c2.js:10:13584)
    at ul (index.e7a183c2.js:10:23347)
    at e.$emit (index.e7a183c2.js:10:28128)
    at o.click (index.e7a183c2.js:86:29785)
    at ul (index.e7a183c2.js:10:23347)
    at HTMLButtonElement.n (index.e7a183c2.js:10:13584)
    at KJ.s._wrapper (index.e7a183c2.js:10:59969)
    at HTMLButtonElement.r (index.e7a183c2.js:29:1711)

happening on https://dandiarchive.org/dandiset/000029/draft which you are welcome to torture directly - it is a test one

[waxlamp]

A newly created contributor doesn't have any affiliation value in it (see https://api.dandiarchive.org/api/dandisets/000029/versions/draft/info/; search for "Turing" to see an example, and compare to the other contributors next to that one).

When I open the Turing contributor in the meditor then click on the plus button for affiliations, I get a console error reporting that this.value is not iterable. Using the debugger reveals that this.value is null in context. I believe that is coming from the default value defined for affiliation being null.

@mvandenburgh, could you confirm my reasoning? If I'm right, then updating the schema to have [] instead of null as the default value there should fix this. A note for completeness: the jsonschema spec says that default values have no restriction but it's "RECOMMENDED" that they validate against the schema they're modifying. null definitely fails that recommendation.

[mvandenburgh]

@mvandenburgh, could you confirm my reasoning? If I'm right, then updating the schema to have [] instead of null as the default value there should fix this. A note for completeness: the jsonschema spec says that default values have no restriction but it's "RECOMMENDED" that they validate against the schema they're modifying. null definitely fails that recommendation.

This is correct. To verify, I manually modified the schema and pointed my local dev instance at it, and it fixed this issue.

[yarikoptic]

In dandischema we have Optional[list]. That's legit. Defaulting to None is legit for Optional. There is a good number of such elements of type array and default null in the jsonschema, not just "affiliation", so fixing for it alone makes little sense to me.

According to above discussion it is valid (RECOMMENDED is not MUST) as far as jsonschema concerned, and such records do pass jsonschema validation, right? If it was not - pydantic export to jsonschema had to be fixed up, but I don't think that is the case here.

So, where is the bug then really which is to be fixed in code? The vue UI library?

[waxlamp]

You're right, @yarikoptic, None is allowed for the specific type used here. But for values that are lists, what is the semantic difference between None and []? More specifically, what is the difference between those values for the list of affiliations for a Person? To me, it seems there is no difference (but that's why I ask). And if that's true, then we really should have List[Affiliation] with a default of [] for the most useful semantics.

If instead we want to keep the type as-is, we'll have to see what we can do in the client. But the error in the meditor occurs inside the framework code, so we'll essentially need to work around the issue in some way. The reason I don't really love that approach is that we would be implicitly treating the None default value as [] -- this seems to indicate that the true type really is List[Affiliation] rather than Optional[List[Affiliation]] (at least in terms of how the meditor approaches Person).

Let me know what you'd like to do here. (And in case you're unaware, there's a parallel discussion happening over at dandi/dandi-schema#282 (comment).)

[candleindark]

There may be another way around this without changing the type or the default value of affiliation in Person.

The generation of JSON schemas for Optional types in the DANDI models is not standard per Pydantic V2. I was requested to put in a customized function to generate JSON schema for Optional types to mimic JSON schema generation by Pydantic V1. If that function is removed, the affiliation property of Person in the JSON schema would be the standard generation by Pydantic V2 as the following.

        "affiliation": {
          "anyOf": [
            {
              "items": {
                "$ref": "#/$defs/Affiliation"
              },
              "type": "array"
            },
            {
              "type": "null"
            }
          ],
          "default": null,
          "description": "An organization that this person is affiliated with.",
          "nskey": "schema",
          "title": "Affiliation"
        }

This standard generation doesn't have the default value and type disagreement issue.

Be aware that if the customized function is removed. All JSON schema generation of Optional types will be affected. I was told to put in that function, specifically for compatibility with GUI I think.

[yarikoptic]

You're right, @yarikoptic, None is allowed for the specific type used here. But for values that are lists, what is the semantic difference between None and []? More specifically, what is the difference between those values for the list of affiliations for a Person? To me, it seems there is no difference (but that's why I ask). And if that's true, then we really should have List[Affiliation] with a default of [] for the most useful semantics.

There is indeed no semantic difference and ambiguity as two different values provide the same semantic. My point is that if we were to address it as changing to [] - IMHO then we should do it for all such encounters, and likely removing Optional altogether to remove such ambiguity. Then it would be for UI facing elements to treat "empty list" as "really there is nothing". Otherwise, we would be back at similar issue either for another similar attribute (or even this one) which would be assigned None since it would remain to be allowed! And then we also need to check if changing dandischema default would be sufficient or we already have fields with 'null' in those fields and requiring metadata migration... (#565 and related)

@candleindark thanks for digging that up. In my view, it just brings us back to us detecting some shortcoming of UI and providing a workaround instead of a proper solution. So, it seems we are making yet another circle to the same problem. IMHO it would be better both short and long term to (at last) address this issue properly by consulting with upstream.

How difficult would be to construct minimal demonstration of the issue for both definitions?

Related, we pin/use

web/package.json: "@koumoul/vjsf": "2.23.3",

There was v3.0.0 in Nov, 2024 "Vjsf 3 is a complete rewrite of the library compatible with Vue 3 and Vuetify 3.". And we have fresh (thanks @waxlamp !)

• Vue 3 migration #2167

So overall question is -- may be that version already addressed it and proper solution for us is really migration to Vue 3 + vjsf 3 ? I guess should be easy to discover if we have minimal reproducer.

[waxlamp]

How difficult would be to construct minimal demonstration of the issue for both definitions?

Here's a codepen that demonstrates the VJSF 2 issue. Note that the top level value is an array, and clicking on the plus button causes the same error as we're seeing in the meditor. If you remove line 6 (or change the default value to []) then it will work. There is a similar issue for the "address" value that exists inside the value you can now add.

I don't think there's any simple workaround for this. The reason we haven't run into the problem before is that we seem to populate most instances of these list-valued metadata items with []. For example, if you create a new Dandiset and then examine its metadata, you'll see that about has a value of [] rather than null. I am not sure where that happens, but it's clearly an explicit setting of the value, since otherwise the default of null would appear there, and we'd have meditor issues with that metadata item. Unfortunately, the affiliation field of Person does not get this explicit setting of [].

So overall question is -- may be that version already addressed it and proper solution for us is really migration to Vue 3 + vjsf 3 ?

Mike and I just looked into this and it turns out that yes, VJSF 3 does solve this problem. So to solve this problem, we can just wait for the Vue 3 upgrade to happen (Mike will have some news about this at Monday's meeting).

My point is that if we were to address it as changing to [] - IMHO then we should do it for all such encounters, and likely removing Optional altogether to remove such ambiguity.

It seems we agree 😄

Upgrading to Vue3 should solve the immediate issue, but I think we should still talk about "fixing" the typing problem in our schema. Just to reiterate: list-valued items should not be Optional unless there is a specific need to distinguish between "list is missing" and "list is present but empty". In our case, I don't believe we have that need. But the nice thing here is that we have time to discuss this and come to a consensus.

[yarikoptic]

• that codepen does not work for me -- can't add a single item and it says 'valid=false' (may be that's actually why)
  
• the goal was to test if the json model @candleindark posted above with explicit null as anyOf would work - it used to not in vjsf 2, but would it in vjsf3? if yes, we could remove the custom json schema conversion hack @candleindark mentions
• in explorations in Remove Optional for list items. dandi-schema#286 (comment) @candleindark demonstrates that whenever we go to linkml , and convert to pydantic, we would still get Optional[list]... so seems if we could just make vjsf work, we better just "leave metadata schema" as is.

[waxlamp]

• that codepen does not work for me -- can't add a single item and it says 'valid=false' (may be that's actually why)

Right, this is meant to demonstrate the problem with a default null value for array values. You can fix this by changing the null to [] in line 6. If you do that, you will find the same problem again when editing the addresses value of the person model that you should now be able to add (and you can fix that second problem in a similar way as the first). The reason we have two array values, one embedded in the other, is just to demonstrate that the problem exists at both top and nested levels.

the goal was to test if the json model @candleindark posted above with explicit null as anyOf would work - it used to not in vjsf 2, but would it in vjsf3? if yes, we could remove the custom json schema conversion hack candleindark mentions

Unfortunately it does not work in VSJF 3. @mvandenburgh has tested this in a local build of dandi-archive (it turns out it's somewhat difficult to create a VJSF 3 codepen to demo the issue); see dandi/dandi-schema#286 (comment).

VJSF could (should?) handle anyOf gracefully, perhaps by requiring a discriminator or something like that, but currently it doesn't. We haven't yet heard back from the devs on the issue Mike filed; perhaps there will be some insight there.

in explorations in Remove Optional for list items. dandi-schema#286 (comment) @candleindark demonstrates that whenever we go to linkml , and convert to pydantic, we would still get Optional[list]... so seems if we could just make vjsf work, we better just "leave metadata schema" as is.

I did an experiment with LinkML to see if I could get rid of that Optional, and I found a way to do it: multivalued slots can be additionally marked as required, which results in dropping the Optional; the difficult part is setting the empty list as a default value for such a field (see the repo itself for full information).

Using that approach, I think we can proceed with correctly typing these array values as List, which should solve many of our problems.

[candleindark]

AFAIK there are limited kind of default values in LinkML (https://linkml.io/linkml-model/latest/docs/ifabsent/). List is not among those. An additional problem is that, for any slot that is marked required, a data instance has to include a value for such a slot. In that sense, there is really no point in having a default value for a required slot.

P.S. I posted a question regarding default value earlier. Upvoting it may give it more attention.

[mvandenburgh]

Unfortunately it does not work in VSJF 3. @mvandenburgh has tested this in a local build of dandi-archive (it turns out it's somewhat difficult to create a VJSF 3 codepen to demo the issue); see dandi/dandi-schema#286 (comment). I

I pushed the local build I used to test this to a branch, if anyone would like to see for themselves.

But yes, if we moved forward with typing these fields as List[...] = [] instead of Optional[List[...]] = None, it would solve all these schema issues we're having with the Meditor.

[waxlamp]

AFAIK there are limited kind of default values in LinkML (https://linkml.io/linkml-model/latest/docs/ifabsent/). List is not among those.

That's correct, but my experiment linked above shows a way to add a default list value to the generated model.

An additional problem is that, for any slot that is marked required, a data instance has to include a value for such a slot. In that sense, there is really no point in having a default value for a required slot.

I agree that required fields must be supplied at instance creation time, but there is definitely a point to including a default value: if the instance constructor omits a required value, the default value is supplied automatically. That means it is no longer required to supply a specific value in every case, but nonetheless the model instance (which does require one) receives one.

This is essentially what is happening now, except that the default value is None, which is what causes all of our problems.

P.S. I posted a question regarding default value earlier. Upvoting it may give it more attention.

I upvoted.

However, it's hard to know when (if ever) the LinkML folks will actually address this, so I would promote my experimental method as a way to unblock us in the meantime. The benefits are many: we get a correctly specified LinkML model (and correctly typed Pydantic models), as well as a working VJSF implementation. The main drawback is that it requires a "build step" hack (but, we would already have a "build step" of running gen-pydantic, and working with cutting edge tech like LinkML requires this sort of workaround fairly often).

[candleindark]

I agree that required fields must be supplied at instance creation time, but there is definitely a point to including a default value: if the instance constructor omits a required value, the default value is supplied automatically. That means it is no longer required to supply a specific value in every case, but nonetheless the model instance (which does require one) receives one.

I think you were talking more generally, but my thinking was shaped by how Pydantic V2 behaves. In Pydantic V2, a field is not required if and only if it has a default value (see https://docs.pydantic.dev/latest/migration/#required-optional-and-nullable-fields).

I followed the steps in the demo, and was able to generate

personinfo_workaround.py

from __future__ import annotations 

import re
import sys
from datetime import (
    date,
    datetime,
    time
)
from decimal import Decimal 
from enum import Enum 
from typing import (
    Any,
    ClassVar,
    Dict,
    List,
    Literal,
    Optional,
    Union
)

from pydantic import (
    BaseModel,
    ConfigDict,
    Field,
    RootModel,
    field_validator
)


metamodel_version = "None"
version = "None"


class ConfiguredBaseModel(BaseModel):
    model_config = ConfigDict(
        validate_assignment = True,
        validate_default = True,
        extra = "forbid",
        arbitrary_types_allowed = True,
        use_enum_values = True,
        strict = False,
    )
    pass




class LinkMLMeta(RootModel):
    root: Dict[str, Any] = {}
    model_config = ConfigDict(frozen=True)

    def __getattr__(self, key:str):
        return getattr(self.root, key)

    def __getitem__(self, key:str):
        return self.root[key]

    def __setitem__(self, key:str, value):
        self.root[key] = value

    def __contains__(self, key:str) -> bool:
        return key in self.root


linkml_meta = LinkMLMeta({'default_prefix': 'personinfo',
     'default_range': 'string',
     'id': 'https://w3id.org/linkml/examples/personinfo',
     'imports': ['linkml:types'],
     'name': 'personinfo',
     'prefixes': {'linkml': {'prefix_prefix': 'linkml',
                             'prefix_reference': 'https://w3id.org/linkml/'},
                  'personinfo': {'prefix_prefix': 'personinfo',
                                 'prefix_reference': 'https://w3id.org/linkml/examples/personinfo'}},
     'source_file': 'personinfo_workaround.yaml'} )


class Person(ConfiguredBaseModel):
    linkml_meta: ClassVar[LinkMLMeta] = LinkMLMeta({'from_schema': 'https://w3id.org/linkml/examples/personinfo'})

    name1: Optional[str] = Field(default=None, json_schema_extra = { "linkml_meta": {'alias': 'name1', 'domain_of': ['Person']} })
    name2: str = Field(default=..., json_schema_extra = { "linkml_meta": {'alias': 'name2', 'domain_of': ['Person']} })
    name3: str = Field(default="Alan Turing", json_schema_extra = { "linkml_meta": {'alias': 'name3', 'domain_of': ['Person'], 'ifabsent': 'string(Alan Turing)'} })
    aliases1: Optional[List[str]] = Field(default=None, json_schema_extra = { "linkml_meta": {'alias': 'aliases1', 'domain_of': ['Person']} })
    aliases2: List[str] = Field(default=..., json_schema_extra = { "linkml_meta": {'alias': 'aliases2', 'domain_of': ['Person']} })
    aliases3: List[str] = Field(default=[], json_schema_extra = { "linkml_meta": {'alias': 'aliases3',
         'domain_of': ['Person'],
         'ifabsent': 'string(aliases3dummy)'} })


# Model rebuild
# see https://pydantic-docs.helpmanual.io/usage/models/#rebuilding-a-model
Person.model_rebuild()

In it, the definition of

    aliases3: List[str] = Field(default=[], json_schema_extra = { "linkml_meta": {'alias': 'aliases3',
         'domain_of': ['Person'],
         'ifabsent': 'string(aliases3dummy)'} })

makes aliases3 not required in the Pydantic level.

Calling Person.model_json_schema() to generate a the JSON schema from the Person model defined in personinfo_workaround.py would result in the following.

personinfo_workaround_modified_pydantic_model.json

{
  "additionalProperties": false,
  "properties": {
    "name1": {
      "anyOf": [
        {
          "type": "string"
        },
        {
          "type": "null"
        }
      ],
      "default": null,
      "linkml_meta": {
        "alias": "name1",
        "domain_of": [
          "Person"
        ]
      },
      "title": "Name1"
    },
    "name2": {
      "linkml_meta": {
        "alias": "name2",
        "domain_of": [
          "Person"
        ]
      },
      "title": "Name2",
      "type": "string"
    },
    "name3": {
      "default": "Alan Turing",
      "linkml_meta": {
        "alias": "name3",
        "domain_of": [
          "Person"
        ],
        "ifabsent": "string(Alan Turing)"
      },
      "title": "Name3",
      "type": "string"
    },
    "aliases1": {
      "anyOf": [
        {
          "items": {
            "type": "string"
          },
          "type": "array"
        },
        {
          "type": "null"
        }
      ],
      "default": null,
      "linkml_meta": {
        "alias": "aliases1",
        "domain_of": [
          "Person"
        ]
      },
      "title": "Aliases1"
    },
    "aliases2": {
      "items": {
        "type": "string"
      },
      "linkml_meta": {
        "alias": "aliases2",
        "domain_of": [
          "Person"
        ]
      },
      "title": "Aliases2",
      "type": "array"
    },
    "aliases3": {
      "default": [],
      "items": {
        "type": "string"
      },
      "linkml_meta": {
        "alias": "aliases3",
        "domain_of": [
          "Person"
        ],
        "ifabsent": "string(aliases3dummy)"
      },
      "title": "Aliases3",
      "type": "array"
    }
  },
  "required": [
    "name2",
    "aliases2"
  ],
  "title": "Person",
  "type": "object"
}

which indicates that aliases3 is not required.

However, since the aliases3 is marked required in

personinfo_workaround.yaml

id: https://w3id.org/linkml/examples/personinfo
name: personinfo
prefixes:
  linkml: https://w3id.org/linkml/
  personinfo: https://w3id.org/linkml/examples/personinfo
imports:
  - linkml:types
default_range: string
default_prefix: personinfo

classes:
  Person:
    attributes:
      # Not required, implicit default value is None.
      name1:

      # Required, but no default specified.
      name2:
        required: true

      # Required, with a default.
      name3:
        required: true
        ifabsent: string(Alan Turing)

      # Not required, implicit default value is None.
      aliases1:
        multivalued: true

      # Required, but no default specified.
      aliases2:
        multivalued: true
        required: true

      # Required, with a dummy default value to be replaced later.
      aliases3:
        multivalued: true
        required: true
        ifabsent: string(aliases3dummy)

. The JSON schema generated from personinfo_workaround.yaml using gen-json-schema personinfo_workaround.yaml is

personinfo_workaround_from_gen-json-schema.json

{
    "$defs": {
        "Person": {
            "additionalProperties": false,
            "description": "",
            "properties": {
                "aliases1": {
                    "items": {
                        "type": "string"
                    },
                    "type": [
                        "array",
                        "null"
                    ]
                },
                "aliases2": {
                    "items": {
                        "type": "string"
                    },
                    "type": "array"
                },
                "aliases3": {
                    "items": {
                        "type": "string"
                    },
                    "type": "array"
                },
                "name1": {
                    "type": [
                        "string",
                        "null"
                    ]
                },
                "name2": {
                    "type": "string"
                },
                "name3": {
                    "type": "string"
                }
            },
            "required": [
                "name2",
                "name3",
                "aliases2",
                "aliases3"
            ],
            "title": "Person",
            "type": "object"
        }
    },
    "$id": "https://w3id.org/linkml/examples/personinfo",
    "$schema": "https://json-schema.org/draft/2019-09/schema",
    "additionalProperties": true,
    "metamodel_version": "1.7.0",
    "title": "personinfo",
    "type": "object",
    "version": null
}

which indicates `aliases3` is required.

In short, the solution solves the problem encountered in the UI, but it introduces a discrepancy in different representations of the of the model, LinkML and Pydantic, that effects validation behaviors.

VJSF could (should?) handle anyOf gracefully, perhaps by requiring a discriminator or something like that, but currently it doesn't. We haven't yet heard back from the devs on the issue Mike filed; perhaps there will be some insight there.

Since the issue originates from VJSF, is it possible to solve it at a stage that is closer to the input to VJSF, without changing the official model at LinkML or the generated Pydantic model? For example, transform a published schema before it is provided to VJSF to define the forms, or I can write a customized JSON schema generator for Pydantic models that can be used to generate a JSON schema then publish the generated schema at a different repo just for the purpose of feeding VJSF. An example of such a generator is at https://github.com/dandi/dandi-schema/blob/782c421e60b5989d3586d9cf89c526b1ac0a6778/dandischema/utils.py#L71-L94.