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

[API Proposal]: Expose JsonContentOfT publicly #60378

Closed
ErisApps opened this issue Oct 14, 2021 · 9 comments
Closed

[API Proposal]: Expose JsonContentOfT publicly #60378

ErisApps opened this issue Oct 14, 2021 · 9 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@ErisApps
Copy link

Background and motivation

The JsonContent<T> is currently marked as internal which makes it impossible to call (without the use of reflection). However, there are cases in which it could be useful to call the constructor on said class directly.
Eg: when constructing a JsonContent body for an instance while leveraging the newly-introduced source-generated JsonTypeInfo<T> for a PATCH API call.

Therefor I propose to mark said partial class as public instead of internal

API Proposal

namespace System.Net.Http.Json
{
-    internal sealed partial class JsonContent<TValue> : HttpContent
+    public sealed partial class JsonContent<TValue> : HttpContent
    {
        ...

        public JsonContent(TValue inputValue, JsonTypeInfo<TValue> jsonTypeInfo)
        {
            _typeInfo = jsonTypeInfo ?? throw new ArgumentNullException(nameof(jsonTypeInfo));
            _typedValue = inputValue;
            Headers.ContentType = JsonHelpers.GetDefaultMediaType();
        }

        ...
    }
}

API Usage

Won't change the usage internally, just exposes the class for use by external consumers.

var jsonContent = new JsonContent<TBody>(inputValue: body, jsonTypeInfo: jsonBodyTypeInfo);

Risks

I don't know why it was marked as internal in the first place. I assume it has a reason, but I can't really come up with one myself as I'm not familiar enough with the repository nor it's underlying architecture/internal workings.

It's not a breaking change at the very least as it's simply an addition to the public API surface. Maybe it affects trimmability though?

@ErisApps ErisApps added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 14, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

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

Issue Details

Background and motivation

The JsonContent<T> is currently marked as internal which makes it impossible to call (without the use of reflection). However, there are cases in which it could be useful to call the constructor on said class directly.
Eg: when constructing a JsonContent body for an instance while leveraging the newly-introduced source-generated JsonTypeInfo<T> for a PATCH API call.

Therefor I propose to mark said partial class as public instead of internal

API Proposal

namespace System.Net.Http.Json
{
-    internal sealed partial class JsonContent<TValue> : HttpContent
+    public sealed partial class JsonContent<TValue> : HttpContent
    {
        ...

        public JsonContent(TValue inputValue, JsonTypeInfo<TValue> jsonTypeInfo)
        {
            _typeInfo = jsonTypeInfo ?? throw new ArgumentNullException(nameof(jsonTypeInfo));
            _typedValue = inputValue;
            Headers.ContentType = JsonHelpers.GetDefaultMediaType();
        }

        ...
    }
}

API Usage

Won't change the usage internally, just exposes the class for use by external consumers.

var jsonContent = new JsonContent<TBody>(inputValue: body, jsonTypeInfo: jsonBodyTypeInfo);

Risks

I don't know why it was marked as internal in the first place. I assume it has a reason, but I can't really come up with one myself as I'm not familiar enough with the repository nor it's underlying architecture/internal workings.

It's not a breaking change at the very least as it's simply an addition to the public API surface. Maybe it affects trimmability though?

Author: ErisApps
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 14, 2021

Why would you need to have access to the JsonContent<TBody>? Wouldn't the JsonContent.Create method cover your use case?

@ErisApps
Copy link
Author

JsonContent.Create doesn't have an overload that accepts a JsonTypeInfo<T> as far as I know. Other than that, in regard to trimming, said method would result in trim warnings as it is annotated with [RequiresUnreferencedCode(HttpContentJsonExtensions.SerializationUnreferencedCodeMessage)]

@eiriktsarpalis
Copy link
Member

Right, so would JsonContent.Create overload for sourcegen not suffice to cover the use case described here?

@ErisApps
Copy link
Author

I honestly didn't think of that myself when I raised this API proposal, but yeah, that would be sufficient for me.

@eiriktsarpalis
Copy link
Member

Great, would it be possible to update the title and OP of the issue so that it reflects this?

@eiriktsarpalis eiriktsarpalis added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Oct 14, 2021
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

This issue has been marked with the api-needs-work label. This may suggest that the proposal requires further refinement before it can be considered for API review. Please refer to our API review guidelines for a detailed description of the process.

When ready to submit an amended proposal, please ensure that the original post in this issue has been updated, following the API proposal template and examples as provided in the guidelines.

@ErisApps
Copy link
Author

Will do so later today.

@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Oct 14, 2021
@eiriktsarpalis
Copy link
Member

Seems to be a duplicate of #51544.

ErisApps added a commit to ErisApps/CatCore that referenced this issue Oct 16, 2021
ErisApps added a commit to ErisApps/CatCore that referenced this issue Oct 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Projects
None yet
Development

No branches or pull requests

2 participants