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

Service fetch: variables are always string, allow access to data as list, dict etc. #3672

Closed
LaurensBurger opened this issue Dec 8, 2023 · 3 comments · Fixed by #3908
Closed

Comments

@LaurensBurger
Copy link
Collaborator

LaurensBurger commented Dec 8, 2023

Thema / Theme

Form designer

Omschrijving / Description

Currently all variables used in service fetch as query/header parameters are string, this does not allow a form builder to retrieve elements from list, dicts etc.

list = ["foo", "bar"]
When using {{ list.0 }} i expect "foo" in my query, but instead i get "["

Workaround currently is to first store list.0 in a separate string variable to be used as "foo" in my request.

@LaurensBurger LaurensBurger added triage Issue needs to be validated. Remove this label if the issue considered valid. enhancement labels Dec 8, 2023
CharString added a commit that referenced this issue Dec 11, 2023
Bluntly casting everything to str gives "lists" like `"[1, 2, 3]"`.
and `{{mylist.0}}` results in `"["`, not `1`.

Unsure if `datetime.*` should be cast to string or kept in tact,
because I don't know exactly what's meant with:

  # Explicitly cast values to strings to avoid localization

which was added in the switch from simple `{ }` interpolation  to the
Django template double bracket `{{ }}`

Maybe it was just to avoid 1.1 -> "1,1"?
I guess neither this `str(datetime.datetime.now())` nor the default that
Django template rendering gives, are the iso form an api probably wants.
But strings don't play well with the `|date` filter in the templates.
@CharString CharString linked a pull request Dec 11, 2023 that will close this issue
@CharString CharString mentioned this issue Dec 11, 2023
@joeribekker
Copy link
Contributor

Refinement:

This needs a stakeholder. We rely on feedback to make this feature better but are not going to do it on our own.

Small discussion about the use case for service fetch (SF):

Joeri: SF should be used a last resort; If a service/usage is crafted multiple times, this should evolve a separate plugin.
Sergei: Same feeling but thinks SF should be cleaned up and polished anyway.
Silvia: Same feeling, mostly worried about how it is used in practise (with great power comes great responsibility ... but not always taken).
Victorien and Vasileios are not familiar enough to share insights.

@sergei-maertens
Copy link
Member

The fix to avoid localization while still allowing template expressions to post-process variables is too complicated to figure out in safe way. The inputs (template context) need to be escaped properly for the context where they will be used (request headers, query params or request URL), but that intrinsically causes str(...) to be called on a value and removes a lot of django template filter capacities (e.g. you can no longer take a datetime and apply a particular formatting through |date:"...".

However, only doing the escaping on the result of the template evaluation does also not work since unsafe characters can be injected and we can no longer see the individual bits that they're made up of.

This implementation is flawed because it was built with python string formatting in mind and not the django template engine, and this is amplified by some variables sometimes being a JSON type (e.g. a date in YYYY-MM-DD format, which is a string) or rich python types (like a date instance) that can be further processed in the template layer. The template syntax was retrofitted, but ultimately it's not a great fit and it creates localization problems too.

@sergei-maertens
Copy link
Member

I had some time to let this sink in, and came to these conclusions:

  • Service fetch has more issues than the templating aspect. Trying to solve this all at once isn't realistic and will require more funding & proper planning
  • we must put the typing issues (str vs date) out of scope. We can now designate certain behaviours as unsupported
  • the problem in this ticket is really blocking some workflows, so a bandaid fix for this is okay

To progress this, let's make the private _recursive_apply a public utility function. To address the str issues, add an extra argument: transform_leaf: boolean = False

You can then update service fetch to call recursive_apply(context, str, transform_leaf=True)

That allows users to access deep nested keys/lists, while making sure we can avoid data localization and can sanitize the data according to the usage context (query, header, path).

We then need to document that a limitation of service fetch is that its context variables are always strings.

vaszig added a commit that referenced this issue Feb 21, 2024
This fix is done deliberately in this way. As mentioned in the ticket
itself, service fetch has more issues than the templating aspect. With
this we allow users to have access to nested values and avoid
localization when the flag transform_leaf is set to True.
vaszig added a commit that referenced this issue Feb 22, 2024
sergei-maertens added a commit that referenced this issue Feb 22, 2024
…etch-allow-access-to-list-dict

[#3672] Allow access to data as list, dict in service fetch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment