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]: Add System.Text.Json.Nodes.JsonArray.Remove(All|Range) #107962

Closed
jasonmalinowski opened this issue Sep 18, 2024 · 5 comments · Fixed by #109472
Closed

[API Proposal]: Add System.Text.Json.Nodes.JsonArray.Remove(All|Range) #107962

jasonmalinowski opened this issue Sep 18, 2024 · 5 comments · Fixed by #109472
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@jasonmalinowski
Copy link
Member

Background and motivation

System.Text.Json.Nodes.JsonArray is a List<T>-like type that represents an array of JSON values; it has Remove(JsonNode) and RemoveAt(int index) methods that let you remove a single element. In a use we had, we needed to trim a number of values out the array, i.e. "please only return the first 10", which required us to either repeatedly call the Remove methods in a loop, or try to create a new array entirely from scratch. The latter was tricky since the nodes have a Parent object pointer, so you can't create a new array without detaching the nodes from the previous array first. See this discussion for various workarounds that were discussed.

The original concern with the workarounds was performance: in my original case since we were potentially trimming a larger array down to a smaller array, we didn't want the overall cost to be above O(n) of the original size of the JSON object. Thus, repeatedly calling RemoveAt() could be O(n^2) unless we were careful to remove from the end in reverse order. Although the workarounds weren't bad, it meant what should otherwise be a simple operation became more complicated.

API Proposal

The following two methods are added

namespace System.Text.Json.Nodes;

public class JsonArray
{
    public int RemoveAll(Predicate<JsonNode> match);
    public void RemoveRange(int index, int count);
}

These signatures mirror the API shape of List<T> so they should be familiar with users of the API. Furthermore, since JsonArray is implemented under the covers as a List<T>, this ensures implementation should be pretty easy too.

API Usage

JsonArray array;

// Let's clip the array to only 10 elements
if (array.Count > 10)
    array.RemoveRange(10, array.Count - 10);

// We'll also remove nested arrays
array.RemoveAll(n => n is JsonArray);

Alternative Designs

No response

Risks

Since I'm proposing to reuse the existing patterns of List<T>, it means this carries along any regret we have of those APIs. I'm not aware of any specific problems with them, but they may of course exist!

@jasonmalinowski jasonmalinowski added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Sep 18, 2024
Copy link
Contributor

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

@eiriktsarpalis
Copy link
Member

Sounds reasonable. One potential alternative that has been partially explored in #76375 is exposing extension methods or DIMs for IList<T> in general that makes it more practical to manipulate. There's other list types in libraries that would benefit from such helpers.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Sep 18, 2024
@eiriktsarpalis eiriktsarpalis added this to the 10.0.0 milestone Sep 18, 2024
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 18, 2024
@eiriktsarpalis eiriktsarpalis self-assigned this Sep 18, 2024
@JakeYallop
Copy link
Contributor

As we are looking at these APIs - do we also want to add APIs to allow slicing in general?

JsonArray array = ...;
var slice = array[5..^1];

@eiriktsarpalis
Copy link
Member

Because JsonNode instances can only be assigned to a single parent node, I don't think it's possible to implement slicing without deep cloning the elements, which would probably violate callers' expectations.

@terrajobst
Copy link
Member

terrajobst commented Sep 24, 2024

Video

  • We should be using Func<T, bool> as that's the standard now the "natural" type for a lambda
  • We can consider DIMs for IList<T> later
namespace System.Text.Json.Nodes;

public class JsonArray
{
    public int RemoveAll(Func<JsonNode, bool> match);
    public void RemoveRange(int index, int count);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 24, 2024
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Sep 25, 2024
karakasa added a commit to karakasa/runtime that referenced this issue Sep 27, 2024
@dotnet-policy-service dotnet-policy-service bot added in-pr There is an active PR which will close this issue when it is merged labels Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json help wanted [up-for-grabs] Good issue for external contributors in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants