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

feat(lib): only warn on missing fields in Jexl dependencies #1096

Closed

Conversation

fkm-adfinis
Copy link
Contributor

@fkm-adfinis fkm-adfinis commented Oct 29, 2020

Currently, the whole execution breaks if a referenced field is missing.

cc @winged

Currently, the whole execution breaks if a referenced field is missing.
@czosel
Copy link
Contributor

czosel commented Oct 29, 2020

Hmm, is this really a good idea? Doesn't the backend fail hard as well in this case?

@anehx
Copy link
Contributor

anehx commented Oct 29, 2020

What is the use case for this? If the assertion throws an error it means that the form is misconfigured - that should result in an error not only a warning..

@winged
Copy link

winged commented Oct 29, 2020

What is the use case for this? If the assertion throws an error it means that the form is misconfigured - that should result in an error not only a warning..

There are multiple issues with assert()ing there:

  • One, it's not easily caught by the people who configured the form in the first place. Case in point, a (real) customer who configures his own form reported "when I add question A to form X, the whole form disappears". The question turned out to reference another one that was missing in form X. Instead of assert(), some noticeable error during configure-time would be preferrable, instead of a "crash" at run-time.
  • Second, it would allow users to reuse questions in multiple forms, by allowing non-existent references to be undefined/None instead, which can then be tested for in the expression.

Due to the difficulty of implementing the first option, I'd prefer a warning logged instead of an outright crash, which is what this PR does.

@winged
Copy link

winged commented Oct 29, 2020

Hmm, is this really a good idea? Doesn't the backend fail hard as well in this case?

I think it would, but it's been a while since I worked on it. But if we can agree on the fix here, it'd be rather easy to implement

@anehx
Copy link
Contributor

anehx commented Oct 29, 2020

Since the backend will throw a runtime error I don't think that this is a good idea at all. The real fix would be to validate the jexl on saving, but that is not really possible since a question doesn't have to be in context of a form.

In other words, this won't fix your problem but only delay it and make it less easy to detect

@winged
Copy link

winged commented Oct 29, 2020

Hmm, is this really a good idea? Doesn't the backend fail hard as well in this case?

I think it would, but it's been a while since I worked on it. But if we can agree on the fix here, it'd be rather easy to implement

I wasn't clear here - I meant it'd be easy to implement in the backend so as not to crash.

Since the backend will throw a runtime error I don't think that this is a good idea at all. The real fix would be to validate the jexl on saving, but that is not really possible since a question doesn't have to be in context of a form.

In other words, this won't fix your problem but only delay it and make it less easy to detect

Agreed, this would need to go in tandem with the corresponding fix in the backend.

The question itself is still valid, but it's inclusion in the form is not. but you're right - even then it's not clear whether the expression will be valid at run-time, as the form coulc be included in another form etc, implying we'd need to validate all the variants etc. Not really viable.

@anehx
Copy link
Contributor

anehx commented Oct 29, 2020

I think this solution won't help at all. Right now, this is not a bug but an incorrect usage of caluma. The real "fix" would IMHO be to have hidden and required jexls on the formquestion that override or extend the questions jexl. This would allow said reusage in different forms which is not supported ATM.

@fkm-adfinis fkm-adfinis marked this pull request as draft October 30, 2020 09:26
@anehx
Copy link
Contributor

anehx commented Nov 17, 2020

As discussed: this is being closed in favor of a new feature #1117

@anehx anehx closed this Nov 17, 2020
@fkm-adfinis fkm-adfinis deleted the feat/jexl-warn-missing branch March 3, 2021 09:59
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.

4 participants