Skip to content

[release/6.0] Fix custom JsonConverterFactories not working with nested type/property declarations. - #62643

Merged
safern merged 1 commit into
release/6.0from
backport/pr-62595-to-release/6.0
Jan 10, 2022
Merged

[release/6.0] Fix custom JsonConverterFactories not working with nested type/property declarations.#62643
safern merged 1 commit into
release/6.0from
backport/pr-62595-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

Backport of #62595 to release/6.0

/cc @eiriktsarpalis

Customer Impact

Customer reported issues in #62079 and #61860. Fixes a bug in which System.Text.Json will generate invalid code in cases where transitive dependencies in the type graph of a serializable type specify custom JsonConverterFactories via the JsonConverter attribute.

Testing

Added regression tests validating the impacted use cases.

Risk

Low to moderate. Makes a minor adjustment in the product code.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #62595 to release/6.0

/cc @eiriktsarpalis

Customer Impact

Testing

Risk

Author:github-actions[bot]
Assignees:-
Labels:

area-System.Text.Json

Milestone:-

@eiriktsarpaliseiriktsarpalis self-assigned this Dec 10, 2021
@eiriktsarpaliseiriktsarpalis added this to the 6.0.x milestone Dec 10, 2021
@eiriktsarpaliseiriktsarpalis added the Servicing-consider Issue for next servicing release review label Dec 10, 2021
@danmoseley

Copy link
Copy Markdown
Contributor

@ericstj could you please handle getting approval for this? by email or on Thurs.


private readonly HashSet<string> _emittedPropertyFileNames = new();

private bool _generateGetConverterMethodForTypes = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: at first it wasn't clear to me that these were tied to the _currentContext and that _currentContext would actually change as the emitter did its work. It might make sense to associate this mutable state to a single object or name it in a way that makes it clear how it applies and when it changes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For example as members of ContextGenerationSpec

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM; allows custom converters for types not adorned with [JsonSerializable]

@ericstjericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Dec 15, 2021
@ericstj

Copy link
Copy Markdown
Member

Approved over email.

@danmoseley

Copy link
Copy Markdown
Contributor

is this ready to merge?

@danmoseley

Copy link
Copy Markdown
Contributor

test failures will be fixed by #63526. this can merge if it's ready.

@danmoseley

Copy link
Copy Markdown
Contributor

Let's re-run CI to verify those are fixed, since it's Sunday..

@ericstj

Copy link
Copy Markdown
Member

@safern for some reason we're seeing PackageValidation fail again like it was here: #59908. I'm not sure why that might have just started happening. Maybe because this is a rather old PR and is running old pipelines?

@eiriktsarpalis

Copy link
Copy Markdown
Member

Maybe it just needs rebasing?

@safern

Copy link
Copy Markdown
Member

@safern for some reason we're seeing PackageValidation fail again like it was here: #59908. I'm not sure why that might have just started happening. Maybe because this is a rather old PR and is running old pipelines?

I've seen it in multiple PRs already, I think I need to service the fix to 6.0 on the SDK and update the package. I'll do that for 6.0.3 as we don't have time for 6.0.2 anymore.

@safern
safern merged commit 7780d28 into release/6.0Jan 10, 2022
@safern
safern deleted the backport/pr-62595-to-release/6.0 branch January 10, 2022 23:34
@ghostghost locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.JsonServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@danmoseley@ericstj@eiriktsarpalis@safern@steveharter