dandi / dandi-schema

Schemata for DANDI archive project
Apache License 2.0
5 stars 8 forks source link

Allow dictionary representation of `Dandiset` to have extra attributes #218

Closed candleindark closed 5 months ago

candleindark commented 5 months ago

This PR allows the dictionary representation of Dandiset to have extra attributes.

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects. While extra attributes can be passed to the resulting Dandiset objects this way, the dictionary representations of these Dandiset objects will not contain the extra attributes unless Dandiset is explicitly configured to allow extra attributes.

dandi-archive requires extra attributes passed in constructing a Dandiset object to be present in the dictionary representation of the object. This change in this PR explicitly configures Dandiset to allow extra attributes to meet this requirement, and meeting this requirement will eliminate some of the failures encountered in https://github.com/dandi/dandi-archive/pull/1823.

Please note, the change in this PR will modify the JSON schemata for Dandiset and PublishedDandiset slightly. Both schemata will contain the additional key of "additionalProperties" with the value of true. Please consider increase the version number of the JSON schemas to 0.6.6.

Note: This PR is a follow up to #203 to make a new version of dandi-schema with Pydantic 2.0 working for dandi-archive.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4423b41) 97.66% compared to head (6ac7c8d) 91.99%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #218 +/- ## ========================================== - Coverage 97.66% 91.99% -5.67% ========================================== Files 18 18 Lines 1798 1799 +1 ========================================== - Hits 1756 1655 -101 - Misses 42 144 +102 ``` | [Flag](https://app.codecov.io/gh/dandi/dandi-schema/pull/218/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dandi/dandi-schema/pull/218/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi) | `91.99% <100.00%> (-5.67%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dandi#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yarikoptic commented 5 months ago

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects.

could you please elaborate/point to what you mean since I find no model_construct in dandi-archive codebase and if there is some custom use like that I wonder if that is something we should aim to avoid.

candleindark commented 5 months ago

Here is the reason why this PR is needed.

Before the deprecation of the json_dict method, the method was a workaround to generate a dictionary representation of DandiBaseModel objects that's JSON serializable. I found that such workaround was not necessary and deprecated it in https://github.com/dandi/dandi-schema/pull/203. However, in resolving the failures in https://github.com/dandi/dandi-archive/pull/1823, I learned that dandi-archive attaches extra attributes to Dandiset objects (extra in the sense that these attributes are not defined as fields in the Dandiset model). To have these extra attributes included in the dictionary representation of the Dandiset objects generated by the facilities provided by Pydantic, the model has to be marked explicitly to allow extra attributes, and the change in this PR is the explicit marking.

candleindark commented 5 months ago

ATM, packages such as dandi-archive construct Dandiset objects using Dandiset.model_construct() to allow extra attributes to be passed to the resulting Dandiset objects.

could you please elaborate/point to what you mean since I find no model_construct in dandi-archive codebase and if there is some custom use like that I wonder if that is something we should aim to avoid.

The two deprecated methods in this line is the cause of the problem.

unvalidated calls model_construct() and json_dict calls model_dump().

yarikoptic commented 5 months ago

@jwodder and @satra please review on either you bless such changes.

jwodder commented 5 months ago

If the JSON Schema has changed, we need to bump the schema version.

yarikoptic commented 5 months ago

we would need to release again , so labeling accordingly

candleindark commented 5 months ago

Once https://github.com/dandi/dandi-schema/pull/221 is merged to master. This PR should be ready for merge.

candleindark commented 5 months ago

@yarikoptic This one is ready to merge.

yarikoptic commented 5 months ago

ok, let's see where it would get us