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

Pony JSON - difficult to modify iso JsonDoc #16

Open
sgebbie opened this issue Nov 17, 2021 · 9 comments
Open

Pony JSON - difficult to modify iso JsonDoc #16

sgebbie opened this issue Nov 17, 2021 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@sgebbie
Copy link

sgebbie commented Nov 17, 2021

Currently json.JsonDoc provides accessors via the data field. However, if one has an iso reference to a JsonDoc that has already been populated, then it is difficult to modify the data after the fact.

Background

For example, the following code will work (where a ref alias is recovered):

be dostuff(jdoc: JsonDoc iso) =>
    let jdoc': JsonDoc ref = recover ref consume jdoc end
    try
        (jdoc'.data as JsonObject).data("seq") = I64(123)
    end

But then jdoc' is no longer iso.

One could replace jdoc.data altogether:

    let jom = recover iso Map[String, JsonType] end
    jom("seq") = I64(123)
    jdoc.data = recover iso JsonObject.from_map(consume jom) end

But then the full map would need to be recreated.

Proposal

Based on feedback from Borja o'Cook we could instead introduce "getters" into JsonDoc and JsonObject (and JsonArray) such that viewpoint adaptation could be applied to help achieve the desired access to the data while satisfying the reference capability constraints.

class JsonDoc
  // ....
  fun get_data(): this->JsonType! =>
    data

// ...

class JsonObject
  // ...
  fun get_data(): this->Map[String, JsonType]! =>
    data

// ---

actor Main
  let _env: Env

  new create(env: Env) =>
    _env = env
    try
      let json_string = "{\"key\":\"value\"}"
      let sendable_doc: JsonDoc iso = recover iso JsonDoc.>parse(json_string)? end
      this.dostuff(consume sendable_doc)
    end

  be dostuff(jdoc: JsonDoc iso) =>
    try
      let new_doc: JsonDoc iso = recover
        let doc_ref = consume ref jdoc
        let obj: JsonObject ref = doc_ref.get_data() as JsonObject
        let data = obj.get_data()
        data("seq") = I64(123)
        doc_ref
      end
      this.morestuff(consume new_doc)
    end

  be morestuff(jdoc: JsonDoc iso) =>
   _env.out.print(jdoc.string(where indent="  ", pretty_print=true))
@sgebbie
Copy link
Author

sgebbie commented Nov 17, 2021

In terms of naming, we could go with: get_data, which is not my favourite, but makes sense. Unfortunately, we can't use data() for backwards compatibility. We could also consider payload() or simply d().

Another option that may work well is apply() so as to leverage the syntactic sugar.

sgebbie referenced this issue in sgebbie/ponyc Nov 17, 2021
This introduces a viewpoint for accessing the internal data for a JSON
element: document, object, array.

This simplifies the process for modifying `iso` instances of `JsonDoc`
after prior creation.

ponylang/ponyc#3922
@sgebbie
Copy link
Author

sgebbie commented Nov 17, 2021

I've started putting together the following PR:

which allows one the write the following:

be dostuff(jdoc: JsonDoc iso) =>
  _seq = _seq + 1 // update the sequence
  let jdoc': JsonDoc iso = recover
    let jdoc_ref = consume ref jdoc
    try
      let m: Map[String, JsonType] ref = (jdoc_ref() as JsonObject)()
      m("seq") = _seq.i64()
    end
    jdoc_ref
end

@ergl
Copy link
Member

ergl commented Nov 17, 2021

@sgebbie Since this changes the standard library, I think this will be required to go through the RFC process.

@sgebbie
Copy link
Author

sgebbie commented Nov 17, 2021

@sgebbie Since this changes the standard library, I think this will be required to go through the RFC process.

Cool, that makes sense. I am just going to add a unit test to my PR and then I will take a look at the RFC documentation.

sgebbie referenced this issue in sgebbie/ponyc Nov 17, 2021
This demonstrates that we can access the internal `data` fields in an
`iso` document, make modifications and recover an `iso`.

ponylang/ponyc#3922
@sgebbie
Copy link
Author

sgebbie commented Nov 18, 2021

I've started drafting and RFC for this change:

I'll send through a PR once I've completed the documentation.

@ergl
Copy link
Member

ergl commented Nov 18, 2021

@sgebbie Cool. One point to consider is that since pony is pre-1.0, we can be more relaxed about breaking changes. In this case, I'd at least write down the option of making the data field private, or of calling the getter data. You can put these alternatives on the "Alternatives" section of the RFC.

sgebbie referenced this issue in sgebbie/ponyc Nov 19, 2021
Here we demonstrate various combinations of access using the `apply()`
sugar or the literal invocation of `apply()`.

ponylang/ponyc#3922
sgebbie referenced this issue in sgebbie/ponylang-rfcs Nov 19, 2021
This sketches the consideration for adding `apply()` methods to JSON,
together with potential alternatives.

ponylang/ponyc#3922
sgebbie referenced this issue in sgebbie/ponylang-rfcs Nov 19, 2021
This sketches the consideration for adding `apply()` methods to JSON,
together with potential alternatives.

ponylang/ponyc#3922
@sgebbie
Copy link
Author

sgebbie commented Nov 19, 2021

I've posted an RFC for this proposed change:

@jasoncarr0
Copy link

jasoncarr0 commented Nov 21, 2021

I'm not going to suggest that we should hold this up for it, but I'm marking this as another use case that is solved very easily by recover blocks with receiver ponylang/rfcs#182

sgebbie referenced this issue in sgebbie/ponylang-rfcs Nov 21, 2021
ponylang/ponyc#3922
sgebbie referenced this issue in sgebbie/ponyc Nov 21, 2021
ponylang/ponyc#3922
@sgebbie
Copy link
Author

sgebbie commented Nov 23, 2021

Following the Pony sync 2021-11-23 it seems like the issue in the JSON API has more to do with the difference in how the compiler is handling field access versus how it is handling method access (with viewpoint adaption).

The suggestion was to rather file a new bug report that isolates an example of this difference in behaviour.

@SeanTAllen SeanTAllen added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Feb 5, 2022
@SeanTAllen SeanTAllen transferred this issue from ponylang/ponyc Feb 6, 2024
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 6, 2024
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants