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

Feature/4980 add json schema to submitted data of json dump plugin #5007

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

viktorvanwijk
Copy link
Contributor

@viktorvanwijk viktorvanwijk commented Jan 9, 2025

Closes #4980

Changes

Added JSON schema definition to the submitted data of the JSON dump registration plugin

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from 0740d05 to 7997531 Compare January 10, 2025 11:21
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch 7 times, most recently from 792dedd to 474286b Compare January 20, 2025 15:36
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 99.36102% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.70%. Comparing base (8c29ae8) to head (54ceb6f).
Report is 20 commits behind head on master.

Files with missing lines Patch % Lines
...penforms/registrations/contrib/json_dump/plugin.py 96.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5007      +/-   ##
==========================================
+ Coverage   96.64%   96.70%   +0.05%     
==========================================
  Files         764      768       +4     
  Lines       26088    26409     +321     
  Branches     3406     3446      +40     
==========================================
+ Hits        25212    25538     +326     
+ Misses        610      607       -3     
+ Partials      266      264       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from 474286b to 919185a Compare January 21, 2025 10:20
@viktorvanwijk viktorvanwijk marked this pull request as ready for review January 21, 2025 10:34
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from 919185a to 27ad453 Compare January 21, 2025 10:37
@viktorvanwijk viktorvanwijk marked this pull request as draft January 21, 2025 11:38
The schema for the registration variables are left out for now, as they can't be selected in the JSON dump plugin options
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch 3 times, most recently from fb890da to 3be5893 Compare January 22, 2025 12:48
…f variables to include

Also updated the JSON dump plugin
- Add test for JSON schemas of formio components
- Add test for form to JSON schema
Also updated form_variables_to_json_schema
…led field

Non-required fields can be empty upon submission, so need to include an empty string.
…t if no value was submitted

If the component is not required, the value is an empty dict, which means there are no required properties.

The `component is None` check is also not necessary anymore, as the user defined variables are excluded earlier already
…omponents

Only add enum/properties to the schema when the data source is not another form variable. If the data source is a form variable, the list of options is not available here, so it doesn't make sense to include the enum/properties in the JSON schema
When the data source for these components is another form variable, the options are not available in the component.as_json_schema methods based on the form alone, because all user defined variables and calculated values are saved in the DB at the moment of submission. So just the form is not enough to get this data, and we need to do some post-processing in the plugin where we have the submission available
In the JSON dump docker app, explicitly load the received data before sending again. This prevents the data from being interpreted as a string instead of a JSON object.
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round, focusing on the architectural aspect and not so much on the actual schemas.

You should probably start from #5007 (comment)

src/openforms/forms/utils.py Outdated Show resolved Hide resolved
Comment on lines 23 to 25
from openforms.variables.registry import (
register_static_variable as static_variable_registry,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

openforms.variables.service.get_static_variables() is public API, but only returns the FormVariable instances and not the static variables around it that have more information.

On the other hand, looking at all possible flavours we have:

  • Formio component -> FormVariable with source set to component (we can get more info from the component)
  • User defined variable -> FormVariable with source set to user_defined (we can't get more info at all, the shittiest of all flavours)
  • Static variables -> FormVariable that's only exists in-memory and not in the DB, but the implementation details are outside of it

I think it would be nice if we can define the schema generation fully in terms of the FormVariable class. It already has some methods/helpers to do some Formio specific stuff, so it's not weird to have it handle the first flavour too.

Roughly I'm thinking:

# easy to extend (future & probably registration variables) generator
def _iter_form_variables(form: Form) -> Iterator[FormVariable]:
    # static variables are always available
    yield from get_static_variables()
    # then handle all form variables holding dynamic data (component + user defined)
    yield from form.formvariable_set.all()

then you can consume that in your top-level function:

def generate_json_schema(...):
    requested_variable_schemas = {
        key: variable.as_json_schema()
        for variable in _iter_form_variables(form)
        if (key := variable.key) in limit_to_variables
    }

    ...  # process this with deep objects etc., the FormioData data structure might be useful here too

Using generators like this is quite elegant, since you decouple the "give me all the variables that can possibly exist for a form" from the actual schema generation and you can do a single dict comprehension instead of having to check limit_to_variables membership for every flavour of variable, which also lowers the chances of forgetting to perform such a check. And, if more flavours of variables come into existence in the future, then it's a matter of updating the generator above and we're set.

src/openforms/forms/utils.py Outdated Show resolved Hide resolved
src/openforms/forms/utils.py Outdated Show resolved Hide resolved
src/openforms/forms/utils.py Outdated Show resolved Hide resolved
Comment on lines 194 to 197
# If the select boxes component is not filled, set required
# properties to empty list
if not values[key]:
schema["properties"][key]["required"] = list()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not send a value at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'not filled' is not the best description. In the actual not-filled case, the value is an object with all the options listed as False. Here, I mean in the case it is hidden. Then it's an empty object, so I remove the required list.

src/openforms/registrations/contrib/json_dump/plugin.py Outdated Show resolved Hide resolved
src/openforms/variables/service.py Outdated Show resolved Hide resolved
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from 3be5893 to 5c4e3aa Compare January 24, 2025 16:37
to_multiple, rewrite_formio_components, and DataContainer need to be used in the JSON dump plugin
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from 5c4e3aa to fa4cbd5 Compare January 27, 2025 13:14
* Rewrite fomio components only once before the processing, as it can be called 10-100 times if done for each component separately.
* Perform component matching based on the dict, as it makes the structure flatter and easier to follow
* Make `post_process` a plain function, as there is not much reason for it to be part of the plugin class. Also, if the base plugin ever gains a `post_process` hook, this would be problematic
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from fa4cbd5 to fb8e6dc Compare January 27, 2025 13:27
…ariables to a JSON schema

* Removed the user defined part, as it is valid for any type of form variable, albeit very simplistic
* Convert to a constant
* Use it as a base schema for all static variables
openforms.forms.utils is import/export stuff, so created a dedicated module json_schema

The idea is to fully define the schema generation in terms of `FormVariable`, as we have three possible options:
* Formio component -> `FormVariable` with source set to component (we can get more info from the component
* User-defined variable -> `FormVariable` with source set to user_defined (we can't get more info at all)
* Static variable -> `FormVariable` that only exists in-memory and not in the DB, but the implementation details are outside of it

Implementation-wise:
* Formio component -> schema will be generated inside `FormVariable`. If it fails, fall back to basic schema based on data type.
* User-defined variable -> use basic schema based on data type
* Static variable -> generate schema separately, and assign it manually the `json_schema` property of a `FormVariable` instance
@viktorvanwijk viktorvanwijk force-pushed the feature/4980-add-json-schema-to-submitted-data-of-json-dump-plugin branch from fb8e6dc to 54ceb6f Compare January 27, 2025 13:37
@viktorvanwijk viktorvanwijk marked this pull request as ready for review January 27, 2025 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JSON Schema definition of the submitted data to the JSON registration backend plugin
2 participants