-
Notifications
You must be signed in to change notification settings - Fork 38
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
Introduce the MISSING sentinel and use it for HA timeout on GroupPolicies #885
Conversation
This is not used anywhere yet.
By defaulting to `MISSING`, we make it possible to treat an explicit `None` as a request for `null` in the payload.
This change updates payload serialization in two significant ways: - MISSING is automatically removed from payloads, and this is applied recursively. This means that it can be included anywhere in a (well-formed) body and the SDK will find and remove it. Custom types inside of the payload could interfere, but native dict/list structures and PayloadWrapper objects are fully supported - PayloadWrapper types are recursively converted to dicts, meaning that it is now valid to construct a helper type based on PayloadWrapper which is nested As a result of this change, simplify the GroupPolicies code to always assign the ha timeout (with a default value of MISSING) and replace the unit test for this behavior with a functional test. The new recursive conversion was benchmarked against a version which was micro-optimized to avoid deep-copying of dict and list data, and found to be nearly 2x *faster* than the optimized version on a typical payload, by virtue of the fact that it did not make unnecessary method calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left feedback but this is great as-is.
authentication_assurance_timeout: int | None = None, | ||
authentication_assurance_timeout: int | ||
| None | ||
| utils.MissingType = utils.MISSING, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. This style runs afoul of my similar preference for implicit string concatenation: in parentheses. I'm surprised that black doesn't modify this already, but I may have to adjust to this style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like black
leaves this in the "user specified style".
I'll change this one -- and BTW I have a much stronger feeling that parenthesized is better here than with the strings.
I'll look into adding a slyp
check for this later.
This is an internal feature of the client class. Entirely independently from the payload filtering process (which needs to handle the complex payload types which may be used), a simple helper for `dict|None` data is used to strip out MISSING from any query params and headers before sending. Testing also confirms the behavior of MISSING on simple JSON and Form-POST data.
There's one additional change here, which is to apply "filter MISSING" behavior to query params and headers, along with a test. |
The commits here reflect a few quick iterative steps I took to get
globus_sdk.MISSING
into place and use it.MISSING
and is internally responsible for stripping it out before the result is passed to other SDK codeMISSING
to be passed around, and strip it out when the payload is actually being prepared for use in the transport layerThis final change makes preparation of payload data the responsibility of the
PayloadWrapper
helper class, and also makes it responsible for recursively transforming anyPayloadWrapper
s to native dicts. Although I was initially concerned that this would introduce a noticeable slowdown, some quick benchmarking revealed this to be very fast on typical data sizes.We could also push the handling of
MISSING
"down the stack" into the transport layer, where it would make sense. But for now, it's the client class' responsibility to prune outMISSING
.In the final state, we should now be able to write client methods like so:
Notably, there is not (yet) any support for use of
MISSING
in query params or other locations. That is intentionally omitted as future work, although it should presumably fit comfortably into this same paradigm.📚 Documentation preview 📚: https://globus-sdk-python--885.org.readthedocs.build/en/885/