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

Make GARE validation as simple as possible #796

Merged

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Aug 6, 2023

This changeset completely replaces #795. I'm closing that as I think this is many times superior.


Rather than trying to build a framework, this is the "no framework" solution, in which we define very concrete helpers for very concrete purposes, including a dedicated one for the "ConsentRequired" Literal.

All of the GARE classes now inherit from an internal helper class, Serializable, which defines from_dict, to_dict, and _supported_fields in a generic way. The last of these does use some signature inspection magic, but nothing too abstruse.

The basic transformation is to replace each combination of a class-level annotation + a SUPPORTED_FIELDS entry with relevant assignment in __init__.

Some tangentially related simplifications and minor improvements are included:

  • extra_fields -> extra

  • Removal of unnecessary str splitting (after object is initialized)

  • isinstance checking can also handle deserialization of dicts

  • checking for non-null values will not accept session_message-only GARE data -- at least one of the semantic fields is required by the rewritten check

  • GlobusAuthRequirementsError does not inherit from GlobusError or Exception -- it is not clear that this inheritance is useful or instructive to any user, since it mixes Exception hierarchies with data-representing hierarchies

  • replace direct ValueError usage with a custom ValidationError class -- this avoids any messy scenarios in which a ValueError is accidentally introduced and incorrectly caught (e.g. from a stdlib call)

There are also some more significant improvements included. Most notably:

  • Annotations explicitly do not accept None unless it is a valid value (i.e. annotations align with validation requirements)

  • to_dict can now look for a concrete type (Serializable) and therefore can automatically invoke to_dict down a tree of objects

Although brevity is a non-goal of this changeset -- more verbose but clearer would be acceptable -- the result is almost 200 LOC lighter in the src/ tree. The primary ways in which things became shorter appear to be:

  • explicit version is often much shorter than the framework-ized version (e.g. LegacyConsentRequiredAPError), and even where the two are close, the explicit version is shorter

  • to_dict and from_dict centralization


📚 Documentation preview 📚: https://globus-sdk-python--796.org.readthedocs.build/en/796/

Rather than trying to build a framework, this is the "no framework"
solution, in which we define very concrete helpers for very concrete
purposes, including a dedicated one for the "ConsentRequired" Literal.

All of the GARE classes now inherit from an internal helper class,
Serializable, which defines from_dict, to_dict, and _supported_fields
in a generic way. The last of these *does* use some signature
inspection magic, but nothing too abstruse.

The basic transformation is to replace each combination of a
class-level annotation + a `SUPPORTED_FIELDS` entry with relevant
assignment in `__init__`.

Some tangentially related simplifications and minor improvements are
included:

- `extra_fields -> extra`

- Removal of unnecessary str splitting (after object is initialized)

- `isinstance` checking can also handle deserialization of dicts

- checking for non-null values will not accept `session_message`-only
  GARE data -- at least one of the semantic fields is required by the
  rewritten check

- GlobusAuthRequirementsError does not inherit from `GlobusError` or
  `Exception` -- it is not clear that this inheritance is useful or
  instructive to any user, since it mixes Exception hierarchies with
  data-representing hierarchies

- replace direct `ValueError` usage with a custom ValidationError
  class -- this avoids any messy scenarios in which a ValueError is
  accidentally introduced and incorrectly caught (e.g. from a stdlib
  call)

There are also some more significant improvements included. Most
notably:

- Annotations explicitly do not accept `None` unless it is a valid
  value (i.e. annotations align with validation requirements)

- to_dict can now look for a concrete type (`Serializable`) and
  therefore can automatically invoke `to_dict` down a tree of objects

Although brevity is a non-goal of this changeset -- more verbose but
clearer would be acceptable -- the result is almost 200 LOC lighter in
the `src/` tree. The primary ways in which things became shorter
appear to be:

- explicit version is often much shorter than the framework-ized
  version (e.g. `LegacyConsentRequiredAPError`), and even where the
  two are close, the explicit version is shorter

- `to_dict` and `from_dict` centralization
sirosen and others added 2 commits August 7, 2023 13:49
…ble.py

Co-authored-by: Ada <107940310+ada-globus@users.noreply.github.com>
Copy link
Contributor

@ada-globus ada-globus left a comment

Choose a reason for hiding this comment

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

Can we make sure this updates the docs to reflect changes to the interfaces? (e.g.,

``GlobusAuthenticationParameters`` classes in an ``extra_fields`` attribute.
)

This makes these more uniform.
@sirosen sirosen merged commit ab6029e into globus:an/add-globus-session-error-support Aug 7, 2023
13 checks passed
@sirosen sirosen deleted the spartan-gare branch August 7, 2023 20:16
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