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

Update JSON builders to provide more mutable/iterable operations #2308

Open
aSemy opened this issue May 18, 2023 · 4 comments
Open

Update JSON builders to provide more mutable/iterable operations #2308

aSemy opened this issue May 18, 2023 · 4 comments
Assignees
Labels

Comments

@aSemy
Copy link
Contributor

aSemy commented May 18, 2023

What is your use-case and why do you need this feature?

The JSON array and object builders are very useful, but their operations are restricted to only add elements. This makes it more difficult to perform additional mutations (like removing elements), or collection-based operations like iterating over the values.

Implementing all of the possible List-specific and Map-specific operations can be easily achieved via delegation.

Missing operations require in unclear workarounds

For example, there's no a clear way of how to remove elements at the moment. Without a default method method, it's easy to

  1. come up with complicated workarounds that aren't easy to read, or scale

    val obj = buildJsonObject {
      put("a", 1)
      put("b", 2)
      put("c", 3)
    }
    println(obj)
    
    println(
      // if I'm in a rush I might quickly hack this, which is confusing
      // and incurs technical/documentation debt
      JsonObject(obj.toMap().run { minus("b") })
    )
    
    println(
      // if I had more time, this is a little more clear
      JsonObject(obj.toMap() - "b")
      // but it requires some deeper understanding how Kotlin/KxS works,
      // more than should be necessary for a basic operation
    )
  2. accidentally write code that looks correct, but introduces bugs

    println(
      // On first glance this looks like it might work
      obj.toMutableMap().minus("b")
      // but it returns a MutableMap, not a JsonObject
    )
  3. or (for Kotlin / KxS beginners) get confused and give up!

Mutable/immutable JSON element mismatch

There is also a mis-match between the JSON builders and the read-only classes they build.

It makes sense to update the JSON builders so they work similarly, which helps with understanding and reduces the difference between the two.

Sharing methods

It would also help understanding if JsonObject/JsonArray shares as much functionality as possible with MutableMap/MutableList, because learning one helps with learning to use the other.

Describe the solution you'd like

  • introduce MutableJsonArray and MutableJsonObject classes
  • MutableJsonArray extends JsonArray and implements MutableList<JsonElement> using delegation
  • MutableJsonObject extends JsonObject and implements MutableMap<String, JsonElement> using delegation
  • Remove JsonArrayBuilder, and replace it with the new MutableJsonArray
  • Remove JsonObjectBuilder, and replace it with the new MutableJsonObject

Example:

public class JsonArrayBuilder @PublishedApi internal constructor(
    private val content: MutableList<JsonElement> = mutableListOf()
): MutableList<JsonElement> by content

I also see two further improvements:

Mutable variants

Currently the JSON object/array builders are internal and are only used for the builder operations, but why is this? Since they would become specialised implementations of a mutable collection, why not make them publicly accessible? I propose the following:

  • Rename JsonArrayBuilder to MutableJsonArray,
  • and JsonObjectBuilder to MutableJsonObject,
  • and update the constructors to be public.

This would allow users to create and define mutable JSON objects and arrays, using a class that would also be shared with the buildJsonObject {} and buildJsonArray {} helpers.

Update extension functions?

If JsonArrayBuilder implements MutableList<JsonElement>, then all of the JsonArrayBuilder extension functions could be adapted to extend MutableList<JsonElement> instead of JsonArrayBuilder.

public fun MutableList<JsonElement>.add(value: Number?): Boolean = add(JsonPrimitive(value))

This would improve the usability of KxS in more situations, making the KxS more convenient to use.

See also

@MqCreaple
Copy link

MqCreaple commented May 31, 2023

Upvote.

I found mutable JSON builders would simplify a lot of code in my project and adding them shouldn't require much work. For now I have to store everything inside MutableList<JsonElement> or MutableMap<String, JsonElement> to make it mutable.

Hope the developers can release the newer package ASAP.

@aSemy
Copy link
Contributor Author

aSemy commented May 31, 2023

After further thought I'm much more keen on the idea of introducing MutableJsonArray and MutableJsonObject classes. This would provide more functionality, and more closely matches how MutableList/List and MutableMap/Map work in the stdlib.

Should I make a new issue for this request @sandwwraith, or can I re-write this one?

TheFruxz added a commit to TheFruxz/Ascend that referenced this issue Jul 7, 2023
via @RefactoringCandidate. This talks about Kotlin/kotlinx.serialization#2308 possible future impact on this function
@zjns
Copy link

zjns commented May 19, 2024

Upvote too, it's very inconvenient to modify json without model class.

@sandwwraith
Copy link
Member

I've returned to this problem briefly, so here are my observations:

  1. Initial code in example can be rewritten simply as JsonObject(obj - "b"), since obj is already a map and there is no need to call toMap() again. Other methods are also available, e.g. obj.filterKeys { !it.startsWith("foo") }. It seems sufficient for 'shallow' object transformation — the only difference with standard collections is that you should call the JsonObject constructor again, which is not that difficult since the compiler will tell you when it is expected.
  2. However, 'deep' transformations, like json["foo"]?.jsonObject?.get("nested")?.jsonObject?.filterKeys { it == "x" }, it is indeed very inconvenient to then assign result back somehow. Making buildJsonObject having MutableMap<String, Element>.() -> Unit lambda type (or making JsonObjectBuilder implement MutableMap) will hardly help here.
    We have the very same problem with collections in stdlib as well — there's no way to edit Map<String, List<List<String>>> nicely.
  3. Making JsonObjectBuilder implement MutableMap may help in cases where you have to do some conditional removal, using this with xxxTo operations, etc:
buildJsonObject {
  addAll(someSource)
  if (someCondition) this["foo"] = JsonPrimitive("bar")
  if (this.size > 10) this.remove("some")
  someCollection.filterTo(this) { ... }
}

This is something that looks nice, but I do not know if this is really in demand. After all, you can do very same things just with put/putAll (since it overwrites the keys) and putAll(x.filter { ... }) instead of removing keys afterwards. Or simply by using buildList/buildMap and converting it to JsonObject later.

  1. Adding MutableMap as a supertype retrospectively to the stable API may bring some undesirable consequences. Take a look at this example:
class X {
    var size = 11

    fun doStuff(): JsonObject {
        return buildJsonObject {
            put("foo", "bar")
            if (size > 10) put("additionalSize", size - 10)
        }
    }
}

Before this change, size would refer to X.size. After the change and recompilation, size would refer to [email protected] — size of the builder. And therefore, code behavior would change. This may introduce very subtle bugs.

  1. Also, making addAll(values: Collection<Number?>) extensions on MutableMap/List wouldn't make sense unless JsonObjectBuilder would also have this supertype — otherwise, it would be impossible to call them.

To conclude, making JsonObjectBuilder implement MutableMap would not solve the biggest pain point (although it may solve several smaller ones) and will bring the big risk of breaking already existing code, so I'm reluctant to make this change. Please let me know what you think of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants