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

Add optField combinator and related #1621

Merged
merged 2 commits into from
Jun 23, 2021
Merged

Conversation

pcapriotti
Copy link
Contributor

This is to work around the lack of expressivity of the alternative instance of Aeson.Parser in terms of error reporting. The fundamental problem is that there is no way to express "unrecoverable" failures, i.e. parsing failure that should always result in a failure for the whole parser, regardless of the existence of alternatives.

For example, let p be a Aeson parser that expects a certain object with fields a and b, q an arbitrary Aeson parser, and consider the parser r = p <|> q. How should r behave exactly in the case where p fails to parse its input?

For example, if the input is not an object, or an object that does not contain the fields a and b, then it makes sense that the parser q is considered. However, if the input is indeed an object that looks like something that p accepts, but nevertheless parsing fails because of some error deep down in the structure of one of its fields, normally we wouldn't want q to be tried at all, and the parse error for p should result in a global failure.

Since this sort of distinction between recoverable and unrecoverable errors is not present at all in Aeson, that means that using the Alternative instance can produce unexpected error messages (as the final error always comes from the last parser tried), and worse than that, result in too lenient behaviour when optional fields are involved.

In this patch, we do not try to solve this problem, but instead work around the leniency issue for optional fields by introducing a new combinator specifically for this case. Specifically, the schema optField name (Just v) sch results in an optional field containing a value corresponding to sch, and which uses the JSON default value v when the haskell value is Nothing.

The corresponding parser does not suffer from the issue above, so it results in a failure whenever the field is present, but its value cannot be parsed by the parser corresponding to sch. Note that specifying Nothing as the default value makes the serialiser simply omit the field altogether when the haskell value is Nothing.

Checklist

  • Title of this PR explains the impact of the change.
  • The description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.

This is to work around the lack of expressivity of the alternative instance of `Aeson.Parser` in terms of error reporting. The fundamental problem is that there is no way to express "unrecoverable" failures, i.e. parsing failure that should always result in a failure for the whole parser, regardless of the existence of alternatives.

For example, let `p` be a Aeson parser that expects a certain object with fields `a` and `b`, `q` an arbitrary Aeson parser, and consider the parser `r = p <|> q`. How should `r` behave exactly in the case where `p` fails to parse its input?

For example, if the input is not an object, or an object that does not contain the fields `a` and `b`, then it makes sense that the parser `q` is considered. However, if the input is indeed an object that looks like something that `p` accepts, but nevertheless parsing fails because of some error deep down in the structure of one of its fields, normally we wouldn't want `q` to be tried at all, and the parse error for `p` should result in a global failure.

Since this sort of distinction between recoverable and unrecoverable errors is not present at all in Aeson, that means that using the Alternative instance can produce unexpected error messages (as the final error always comes from the last parser tried), and worse than that, result in too lenient behaviour when optional fields are involved.

In this patch, we do not try to solve this problem, but instead work around the leniency issue for optional fields by introducing a new combinator specifically for this case. Specifically, the schema `optField name (Just v) sch` results in an optional field containing a value corresponding to `sch`, and which uses the JSON default value `v` when the haskell value is `Nothing`.

The corresponding parser does not suffer from the issue above, so it results in a failure whenever the field is present, but its value cannot be parsed by the parser corresponding to `sch`. Note that specifying `Nothing` as the default value makes the serialiser simply omit the field altogether when the haskell value is `Nothing`.
libs/schema-profunctor/src/Data/Schema.hs Outdated Show resolved Hide resolved
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Looks good! This subtlety could perhaps be added to the example README for schema-profunctor as a suggested pattern. (maybe re-use part of your PR description for that).
Fine to do that as a follow-up PR also if you don't want to wait for a CI re-run.

@akshaymankar akshaymankar merged commit e9f2183 into develop Jun 23, 2021
@akshaymankar akshaymankar deleted the pcapriotti/optional-fields branch June 23, 2021 14:30
pcapriotti added a commit that referenced this pull request Jun 23, 2021
This adds some extra explanations to the section about optional fields, and introduces the new `optField` combinator, added to work around the parser leniency issue described in #1621.
This was referenced Jun 23, 2021
pcapriotti added a commit that referenced this pull request Jun 24, 2021
* Document optField combinator.

This adds some extra explanations to the section about optional fields, and introduces the new `optField` combinator, added to work around the parser leniency issue described in #1621.

* Update CHANGELOG
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.

3 participants