StackOverflow when calling JSON#forValue() with unregistered non-Bean type

[rherrmann]

I got a StackOverflow exception in an attempt to serialize an object that has a property of type Locale with with Svenson 1.4.3.

Unfortunaltely I wasn't able to try the same scenario with the latest svension version. Please ignore if this is already known.

The snippet below can be used to reprduce thi sissue:

public class SvensonBug {

  @Test
  public void serializeLocale() {
    LocaleData localeData = new LocaleData();
    localeData.setLocale( Locale.ENGLISH );

    String string = new JSON().forValue( localeData );

    assertEquals( string, "{\"locale\":\"en\"}" );
  }

  public static class LocaleData {
    private Locale locale;

    public Locale getLocale() {
      return locale;
    }

    public void setLocale( Locale locale ) {
      this.locale = locale;
    }
  }

}

[fforw]

It's not really a bug as just unsupported. Locale is not a one of the types svenson can handle per default. You'd need a JSONifier or provide an alternative getter that converts Locale into supported types / string and make the original getter @JsonProperty(ignored= true)

[rherrmann]

Thanks for the hint, I've solved my problem already. The purpose of this bug report was rather to save others from falling into the same trap.

If svenson cannot handle something - which is perfectly valid - it should clearly state so, i.e. throw an approropriate exception. Causing a StackOverflowError by chance is certainly not a good choice.

[fforw]

Preventing stack overflows would mean costly Java bean cycle detection etc. An option might be special cased blacklisting of certain classes known to be problematic.

[fforw]

The StackflowError also does not happen by chance / randomly. It either happens all the time or none of the time depending on the types involved.

[rherrmann]

Sure, the StackOverflowError happens all the time. What I meant is: it is an implementation detail, and rather an leaking internals to the outside you should separate the API from the internals.

This will allow you to provide a stable API while having the freedom to change the implementation without disrupting clients.

[fforw]

If there was a cheap and easy way I would totally agree with you. As it is, the cost/benefit analysis seems to come out in favor of not checking. Svenson is just what it is. it was never meant to be able to just work with any class.

If you want, you can whip up something in that direction so we can checkout how much of a performance penalty it really is.

[fforw]

I didn't mean to be rude there. There just seem to be different opinions about what kind abstraction svenson is or is supposed to be.

Sure in an ideal world, we would all create non-leaky abstractions all day long and often this is really possible within reasonable bounds. I'm not sure that is the case here.

Apart from the performance aspects there is another issue. I created svenson with a certain vision about how I want it to be working. How to navigate the mismatch between strong Java typing and JSON minimal value typing.

Giving these visions, svenson is opinionated in places and actually wants you do to things a certain way. The biggest obstacle I see in the support for more conversation or loop avoidance by special casing is that I don't see any clear favorites among the many ways of implementing it. Me following my taste would force everyone to follow that decision. With each decision the overall usefulness of svenson decreases.

It often seemed better to provide mechanisms to let the library user decided how to deal with locale, dates etc themselves.

Maybe there's a case for just black-listing common Java types known to only lead to errors.

[rherrmann]

No offence. None taken. This issue is not about 'leaky abstractions', it is about a basic requirement of API which should have safe boundaries and be safe-to-use from its cleints POV. This includes checking parameters and failing with a clear exception message.

To given an example: Integer.parseInt( "foo" ); won't end up somewhere with a stacktrack deep in its internals - it will raise a NumerFormatException that clearly says 'foo is not a number'.

I don't see why having safe boundaries contradicts with 'svenson being opinionated in places and actually wants you do to things a certain way'. Document the 'certain ways' and provide an API that keeps clients on the 'right track'.

Regarding the 'performance aspects', have you actually implemented a check and verified what performance impact it has - or is this another premature optimization?

This is my bit of advice, but end it is your project, so do (or leave undone) what you please.

[fforw]

I've had some optimization rounds with svenson both in terms of performance as well as memory requirements. I initially was sceptical, too, and followed the old adage about premature optimization.

When I implemented the caching of type knowledge within the ObjectSupport system, the gains were something in the range of 30% for mass bean parsing, which was much more than I thought.

I wouldn't really be surprised if loop checking had about the same impact on the generating side.

[fforw]

Released 1.5.1 with a fix aimed at improving the situation. It causes the StackOverflow to be wrapped in a RuntimeException which makes the original caller readable in the stacktrace thus making the whole issue far less annoying.