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

Support suppressed property when deserializing Throwable #3179

Merged
merged 4 commits into from
Jun 30, 2021

Conversation

klaasdellschaft
Copy link
Contributor

Closes #3177

When this solution for #3177 is OK, I would suggest to continue refactoring ThrowableDeserializer.deserializeFromObject(...) such that code duplications are removed / the method gets shorter. For example, the following snippet exists twice in the method

// any pending values?
if (pending != null) {
    for (int i = 0, len = pendingIx; i < len; i += 2) {
        prop = (SettableBeanProperty)pending[i];
        prop.set(throwable, pending[i+1]);
    }
    pending = null;
}

Supporting the suppressed property when deserializing Throwable
@cowtowncoder
Copy link
Member

Sounds good. I will be out of town over the weekend, but will get back to this next week.

One thing I'd need (unless it's already done) to eventually merge the PR is CLA:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

which only needs to be sent once and is good for all Jackson contributions.
It's usually done by printing, filling & signing, scanning/photo, email to info at fasterxml dot com.
(digital version is fine too, wrt modified pdf).
No hurry, only needed after review, but I'll mention it now.

// Maybe it's "suppressed"?
final boolean isSuppressed = PROP_NAME_SUPPRESSED.equals(propName);
if (isSuppressed) {
suppressed = p.readValueAs(Throwable[].class);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling via JsonParser, should go through DeserializationContext; this will keep current context and settings (JsonParser route will create new context with possibly differing settings -- at least until Jackson 3.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, changed the corresponding line to

suppressed = ctxt.readValue(p, Throwable[].class);

@cowtowncoder
Copy link
Member

@klaasdellschaft Aside from one minor comment, looks good (assuming it works). Are changes needed?
If not, I'd just need the CLA and would be happy to merge this and see about refactoring you suggested.

Addressing review comment.
Small refactoring in order to reduce code duplication.
@klaasdellschaft
Copy link
Contributor Author

CLA is sent, and a small refactoring is applied such that duplicated code blocks are removed. From my point of view, ready for merging.

@klaasdellschaft klaasdellschaft marked this pull request as ready for review June 28, 2021 10:38
@cowtowncoder cowtowncoder changed the title Supporting the suppressed property when deserializing Throwable Support suppressed property when deserializing Throwable Jun 30, 2021
@cowtowncoder cowtowncoder added this to the 2.13.0 milestone Jun 30, 2021
@cowtowncoder cowtowncoder merged commit a05e2cb into FasterXML:2.13 Jun 30, 2021
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.

Support suppressed property when deserializing Throwable
2 participants