Skip to content

Correct json for nested objects#126

Merged
dwijnand merged 4 commits intosbt:masterfrom
yarosman:master
Jul 26, 2018
Merged

Correct json for nested objects#126
dwijnand merged 4 commits intosbt:masterfrom
yarosman:master

Conversation

@yarosman
Copy link
Copy Markdown
Contributor

Now if have the next keys

buildInfoKeys := Seq[BuildInfoKey.Entry[_]](
  "build_number" -> version.value,
  "git" -> Map(
    "commit" -> git.gitHeadCommit.value,
    "commit_date" -> git.gitHeadCommitDate.value,
    "tags" -> git.gitCurrentTags.value,
    "commit_branch" -> git.gitCurrentBranch.value
  )
)

we will get the next json:

{"build_number" : "1.0-SNAPSHOT", "git" : "Map(commit -> Some(67858ae976394cfd6dfe27336905458a8412d383), commit_date -> Some(2018-05-15T15:48:43+0300), tags -> List(), commit_branch -> develop-commons)"}

So we have wrong representation for nested Map object.
This merge request allow to generate more correctly json for nested objects.

Copy link
Copy Markdown
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR, @yarosman!

I have a concern around the change to matching Option, and a (less important) idea around matching Map.

But other than that this looks good to go to me!

| private def toJsonValue(value: Any): String = {
| value match {
| case elem: Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]")
| case elem: Option[_] => elem.map(toJsonValue).orNull
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should return null. I think returning "null" was correct. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree, we try to convert to correct json type and null is not "null".
For example, we parse BuildInfo.toJson and instead of getting for some value Option[String] we get String.

https://www.json.org/
"A value can be a string in double quotes, or a number, or true or false or null, or an object or an array."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. We are converting to the correct JSON type, but the string representation of that type.

In that case string representation of JSON null is "null", no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At final we get something like that

{
  "build_number": "1.0-SNAPSHOT",
  "git": {
    "commit": null,
    "commit_date": "2018-05-15T15:48:43+0300",
    "tags": [
      
    ],
    "commit_branch": "develop"
  }
}

Copy link
Copy Markdown
Member

@dwijnand dwijnand May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that works because (on line 73) the null you're returning right now is passed to String#+

And "commit" + ":" + null == "commit:null".

So it's kind of "accidentally doing the right thing". But I'm fairly certain we be returning "null" here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I get null on line 71 where we work with Option.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's the code the produces the null from the option and gets returned by the call to toJsonValue.

The code calling that toJsonValue is the code at line 73, which is consuming the map you described in the PR description: #126 (comment)

It's only because your passing it to String#+ that it works.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I missed something. Can you explain by example, why you want to return "quoted null"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to return the string representation of JSON null.

Here, these examples might help:

  • toJsonValue(Some(123)) should return "123"
  • toJsonValue(Some("abc")) should return "\"abc\"" (quoted string)
  • toJsonValue(None) should return "null"

Another addition we could do is make toJsonValue(null) return "null".

Sorry, the fact that the code is recursive and there's multiple levels of strings makes this hard to communicate. 😄

| value match {
| case elem: Seq[_] => elem.map(toJsonValue).mkString("[", ",", "]")
| case elem: Option[_] => elem.map(toJsonValue).orNull
| case elem: Map[String, Any] => elem.map {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Minor - feature creep): We could also match on Map[String, Any] and sending the key through toJsonValue here. IIUC

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can do it

@yarosman
Copy link
Copy Markdown
Contributor Author

Pull request updated ))

Copy link
Copy Markdown
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks @yarosman.

@dwijnand dwijnand merged commit c686647 into sbt:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants