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

remove OptionalValueException? #4972

Open
sanderr opened this issue Oct 12, 2022 · 4 comments
Open

remove OptionalValueException? #4972

sanderr opened this issue Oct 12, 2022 · 4 comments
Labels
compiler discussion There is a discussion happening on this issue
Milestone

Comments

@sanderr
Copy link
Contributor

sanderr commented Oct 12, 2022

Make sure to read comments below, ticket description has not been updated

Currently an OptionalValueException is raised when an optional relation attribute is accessed that has not received a value or has received an explicit null value. This is inconsistent with the behavior for nullable plain attributes and that for multi relations, which result in null and [] respectively. The goal of this ticket is to investigate the feasibility and discuss the value of dropping the OptionalValueException and returning null instead. When attempting to access an attribute/relation of the null relation, an OptionalValueException should still be raised.

As a next step, we could consider deprecating is defined in favor of != null / != [].

@sanderr sanderr added compiler discussion There is a discussion happening on this issue suggestion Suggest a change, open to discussion labels Oct 12, 2022
@sanderr sanderr changed the title remove OptionalException remove OptionalValueException Oct 12, 2022
@sanderr sanderr changed the title remove OptionalValueException remove OptionalValueException? Oct 18, 2022
@sanderr sanderr self-assigned this Oct 18, 2022
@sanderr
Copy link
Contributor Author

sanderr commented Oct 26, 2022

A non-exhaustive (exhaustive within the set of repos checked out on my system) overview of how this would affect other projects (assuming we keep the exception class around):

  • changes Jinja's is defined behavior: null is considered defined, while the OptionalValueException was not. For models to be compatible with both old and newer versions of core, they would need something like x.y is defined and x.y is not null.
  • The lsm module catches the exception in some places. Mostly this is just to convert it to None (the change would be compatible with those) but in some cases the logic really branches on it.
  • Same for the yang module.
  • std uses it in compatible ways (except for the Jinja part)
  • openstack uses it in compatible ways
  • internal docs uses it in an example
  • pytest-inmanta uses it in a test module (module used to test pytest-inmanta, not something used by the actual pytest plugin)

So the main consequences are that lsm and yang need minor changes, as well as any module that uses is defined on optional relations. These changes could be made in backwards compatible ways so that we have the following compatibility constraints:

  • iso6 requires patched versions of lsm, yang and other relevant modules
  • iso5- is compatible with module versions before and after the patch

@sanderr
Copy link
Contributor Author

sanderr commented Oct 26, 2022

Concluding: it's defintely not trivial and it may cause some fallout but it should be doable. Question is, do we want it sufficiently badly to do it now, before the iso6 release, or do we keep it for iso7?

@sanderr sanderr removed the suggestion Suggest a change, open to discussion label Oct 26, 2022
@sanderr sanderr removed their assignment Oct 26, 2022
@sanderr sanderr self-assigned this Nov 8, 2022
@sanderr
Copy link
Contributor Author

sanderr commented Nov 8, 2022

Planning meeting conclusion: delay this until a couple of months before the next major, migrate modules before that. I will create tickets for this and put them in the appropriate epics.

@sanderr
Copy link
Contributor Author

sanderr commented Nov 22, 2022

I will create tickets for this and put them in the appropriate epics.

inmanta/lsm#546
inmanta/infra-tickets#170
kept this one for the main change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler discussion There is a discussion happening on this issue
Projects
None yet
Development

No branches or pull requests

1 participant