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

[Key Vault] Revise administration models #19049

Closed
wants to merge 2 commits into from

Conversation

mccoyp
Copy link
Member

@mccoyp mccoyp commented Jun 2, 2021

Resolves #13575 and resolves #18992.

@mccoyp mccoyp added KeyVault Client This issue points to a problem in the data-plane of the library. labels Jun 2, 2021
@mccoyp mccoyp requested a review from chlowell June 2, 2021 19:58
if error is None or error.code is None:
self._error = None
else:
self._error = ODataV4Format(vars(error))
Copy link
Member Author

Choose a reason for hiding this comment

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

Small but important detail here: ODataV4Format expects the inner error to be labeled as innererror, but the error we get back labels it as inner_error and doesn't get applied to the resulting ODataV4Format object. Does it make sense to edit the label to be innererror, or would that be forcing a square peg into a round hole?

Copy link
Member

Choose a reason for hiding this comment

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

ODataV4Format expects JSON deserialized to a dict but the Error class generated by autorest has already deserialized the JSON and Pythonified "innererror" to "inner_error". What do you get from error.as_dict()? Also, ODataV4Format.innererror should be a JSON dict as well--it's JSON dicts all the way down.

Copy link
Member Author

Choose a reason for hiding this comment

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

error.as_dict() gives the same dictionary as vars(dict) -- we're still left with an inner_error key 😕

Copy link
Member

Choose a reason for hiding this comment

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

Can you use the key_transformer argument to rename it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay nice, that does work with something like:

def my_key_transformer(key, attr_desc, value):
    if key == "inner_error":
        return ("innererror", value)
    return (key, value)

for errors with no inner error (error.inner_error = None) and errors with JSON dict inner errors (they get recursively renamed)

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like something like this would work as well:

error.as_dict(key_transformer=lambda k, _, v: ("innererror", v) if k == "inner_error" else (k, v))

@mccoyp
Copy link
Member Author

mccoyp commented Jun 7, 2021

Closing, since other changes that affect these are being done in #19099.

@mccoyp mccoyp closed this Jun 7, 2021
@mccoyp mccoyp deleted the admin-models branch June 7, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Successful restore operations may yield error Revise administration models
2 participants