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

Make it easier to get json property values #28588

Closed
Tratcher opened this issue Jan 31, 2019 · 7 comments
Closed

Make it easier to get json property values #28588

Tratcher opened this issue Jan 31, 2019 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json
Milestone

Comments

@Tratcher
Copy link
Member

RE: dotnet/aspnetcore#7105 (comment)

Newtonsoft JObject .Value<string>(propertyName); as a convenient way to find a property and extract the value in a single operation. Note this returns null of the property isn't found. It also type converts the various datatypes to strings if that's the T you requested.

The equivalent code for JsonDocument looks something like this:

document.RootElement.TryGetProperty(key, out var value) ? value.ToString() : null;
@ahsonkhan
Copy link
Member

cc @bartonjs

@bartonjs
Copy link
Member

bartonjs commented Feb 2, 2019

I'm not a fan of it, since it collapses the "undefined/not-present" and "present but null" states. Rather than "because we used to" I guess I'd want to know why such a behavior is desired.

The type coercion also seems wrong to me.

If what's really wanted is only coercion to string, then maybe that's just ToString(string propertyName), though that's a weird overload semantic... so we'd need to name it something like GetPropertyString(string propertyName), which we could say returns null for undefined (and "present but null" would presumably return string.Empty to make this equivalent to the suggested line of code?)... but I'm really missing the use case... so it seems like a thing rare enough to say that writing the long form here is better than adding a method on the type.

@Tratcher
Copy link
Member Author

Tratcher commented Feb 2, 2019

@bartonjs we already have 15+ occurrences of this in AspNetCore Authentication so we added an extension method for this. We were not going to write TryGetProperty(key, out var value) ? value.ToString() : null; eveywhere.

The coercion aspect was less common, frequently the values already were strings.

@bartonjs
Copy link
Member

bartonjs commented Feb 2, 2019

we already have 15+ occurrences of this in AspNetCore Authentication so we added an extension method for this.

That's a small number for Ctrl+V (though I admit I'd make a private non-extension utility method for it if it was all in one assembly and it was me). It also doesn't answer the why question. ("Why" is important to understand if it's an API that should be public, and that it has the right name/semantics/etc.)

@ahsonkhan ahsonkhan assigned ahsonkhan and unassigned ahsonkhan Feb 14, 2019
@joshfree
Copy link
Member

@bartonjs / @rynowak PTAL

@bartonjs
Copy link
Member

(Summarizing things from a couple of different channels)

There are a couple of different interpretations of what is lacking (and therefore what to do):

  • Possible Problem 1: "It's too wordy to look for an optional property and get its value".
    • Code, approx: int? age = element.TryGetProperty("age", out JsonElement ageElem) && ageElem.Type != JsonValueType.Null ? (int?)ageElem.GetInt32() : (int?)null;
    • Solution: Add overloads with the word Property on them. Presumably there are ReadOnlySpan<char>, ReadOnlySpan<byte>, and string version of each:
      • public int GetInt32Property(string propertyName) => elem.GetProperty(propertyName).GetInt32();
      • public bool TryGetInt32Property(string propertyName, out int? value)
        • Needs to determine the truth table for missing, present-but-null, present-but-not-null-or-number, present-but-not-int32, present. (e.g. false/null, true/null, throw, ???, true/value; or true/null, true/null, throw, false/null, true/value).
  • Possible Problem 2: "It's too wordy to get the value from the current element" (non-generic)
    • Solution: Add the explicit conversion operators.
      • They were present in the API proposal but then removed during API review. We'd want a "stronger" request to bring them "back".
  • Possible Problem 3: "It's too wordy to get the value from the current element" (generic)
    • Solution: Add public T GetValue<T>().
      • This is a runtime-constrained open-generic, which we either don't have any BCL examples of, or there at least aren't many. (Only bool, DateTime, DateTimeOffset, decimal, double, float, Guid, int, long, string, uint and ulong are valid.)
  • Possible Problem 4: "It's too wordy to be appropriately flexible"
    • This is value coercion. E.g. that GetInt32 should parse when it's a String, or GetString() should return the raw input when the type isn't String (since it isn't String no unescaping would need to be done).
    • The framework being rigid but configuration systems which want to accept foo: "123" and foo: 123 as the same thing feels like a better answer (to me). It's generally easier to build a flexible system on top of a rigid system than a rigid system on top of a flexible one (see also Javascript == vs ===).
    • For the specific requested case, the ideal answer is probably internal static string Stringify(this JsonElement elem) => elem.Type == JsonValueType.String ? elem.GetString() : elem.GetRawText(); (though, admittedly, the specific requested case wants the version of Stringify that takes a property name) -- and, perhaps JsonValueType.Null wants to be (string)null to distinguish it from the string containing only 4 characters (after unescaping), n-u-l-l.
    • New methods, e.g. GetInt32Coercive, could solve this problem, where the intent to be coercive was caller-indicated.

While a caller can't (in C#) define their own conversion operators for the 12 types that are supported, the other solutions are all doable as user defined extensions.

Given where we are in the release, that nothing is really blocked functionality-wise, and that this issue is potentially asking for one of 16 solutions (bitvector of 4 interpretations), closing the issue seems most appropriate. If there's strong demand for any specific parts it can always be reconsidered in future versions -- but those are best addressed in specific issues, so they can track through discussion / proposal / API review individually.

@davidfowl
Copy link
Member

@bartonjs why close? Why not moved to future?

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

6 participants