vert-x / mod-lang-groovy

Vert.x 2.x is deprecated - use instead
https://github.com/vert-x3/vertx-lang-groovy
Other
16 stars 15 forks source link

Converting GStrings to JSON should yield a String #59

Closed danthegoodman closed 10 years ago

danthegoodman commented 10 years ago

When using Jackson to convert json, GStrings yield unhelpful maps instead of strings.

[message: "Hello $name"]

Actual Result:

{
  "message": {
    "values": ["name"], 
    "strings": ["hello", ""], 
    "valueCount": 1 
  }
}

Expected Result:

{"message": "Hello Bob"}

Original Thread: https://groups.google.com/d/msg/vertx/vP_RyujCFmw/GcZ9rjEdZrQJ

LostInBrittany commented 10 years ago

As the problem was sendi ng GStrings in EventBus, I've modified the Groovy EventBus to deal with GString, as done with Groovy Lists or Maps.

I've also added Unit Tests

Normally your use case should work now, if not please reopen it :)

nscavell commented 10 years ago

Actually I think you need to go through each value if it's a List or Map and convert all values. Something like https://gist.github.com/nscavell/1de7efe090aaa8aeb665.

LostInBrittany commented 10 years ago

Oups, very true! My hack only work for "simple" GString messages. I week correct it tonight! On Feb 5, 2014 6:08 PM, "Nick Scavelli" notifications@github.com wrote:

Actually I think you need to go through each value if it's a List or Map and convert all values. Something like https://gist.github.com/nscavell/1de7efe090aaa8aeb665.

Reply to this email directly or view it on GitHubhttps://github.com/vert-x/mod-lang-groovy/issues/59#issuecomment-34211598 .

danthegoodman commented 10 years ago

@LostInBrittany You could also tell Jackson how to convert GStrings. That would involve hooking up to the Jackson ObjectMapper inside org.vertx.java.core.json.impl.Json, so a fix would have to be made within Vertx. I'm not sure which approach would be more performant.

nscavell commented 10 years ago

vertx core knows nothing about groovy, this needs to be handled by the lang module.

danthegoodman commented 10 years ago

@nscavell Right, sorry, I meant that Json would need a method that would expose the ability to add configuration to the object mapper. Once that had been taken care of, the lang module could call said method and add the configuration.

normanmaurer commented 10 years ago

I don’t think this would be a good idea as Jackson is an implementation detail here..

--  Norman Maurer

An 5. Februar 2014 at 20:12:03, Danny Kirchmeier (notifications@github.com) schrieb:

@nscavell Right, sorry, I meant that Json would need a method that would expose the ability to add configuration to the object mapper. Once that had been taken care of, the lang module could call said method and add the configuration.

— Reply to this email directly or view it on GitHub.

LostInBrittany commented 10 years ago

I agree with @danthegoodman ,I fail to see how I can use the Jackson solution touching only the lang module, as org.vertx.java.core.json.impl.Json hasn't hooks. So if we don't want to touch vert.x main project (and agree with @normanmaurer, we don't want :) ), the only solution I see is @nscavell proposition. Going to add it...

danthegoodman commented 10 years ago

@normanmaurer Understandable.

@LostInBrittany I agree.

LostInBrittany commented 10 years ago

I've followed @nscavell advise for dealing with GStrings inside maps and lists, added new unit tests, (and committed, tested, and merged :) ).

The problem should be solved, this time :) If it isn't, please tell me.