FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

Pretty-printing not working Ion/text backend #245

Closed cowtowncoder closed 3 years ago

cowtowncoder commented 3 years ago

Looks like pretty-printing has not been implemented for textual format.

Note: also need to verify that binary backend will just ignore pretty-printing altogether.

cowtowncoder commented 3 years ago

Looks like this has to be done by using Ion's IonTextWriterBuilder -- I am not quite sure how that is related to IonSystem (seems like latter uses former), since it would seem like IonSystem ought to have a mechanism to create one but looks it doesn't... Some example code is here:

https://amzn.github.io/ion-docs/guides/cookbook.html#pretty-printing

Anyway, I think that:

  1. Probably cannot support general PrettyPrinters, only "default" pretty printer -- not sure whether to error on attempts to assign custom one, or just make non-null impl turn on default pretty printing
  2. Binary writer should ignore any attempts, anyway

I can try tackling (2), but probably need to let someone else with more time do (1): it might not be lots of work, just requires some digging into ion-java API.

cowtowncoder commented 3 years ago

Another note: current IonGenerator instances do not know if they are textual or binary ones; this information should probably be passed and retained to allow binary writers to ignore all pretty-printing.

jhhladky commented 3 years ago

May not be super helpful, but when I need non-IonText output from Ion, I use the facilities already baked into IonJava. For example, I've used the following pattern a lot:

try (OutputStream stream = Files.newOutputStream(...);
     IonWriter writer = <custom configured writer>)
{
  ionObjectMapper.writeValue(value);
}

Where <custom configured writer> might be

IonTextWriterBuilder.pretty()
  .withLongStringThreshold(100).build(stream)

or

IonTextWriterBuilder.json().build(stream)

In a larger sense, I would just call out that there's a fair amount customization already, so it might not be that valuable getting it working through "official" means, esp if those same configuration options can't be easily exposed.

cowtowncoder commented 3 years ago

@jhhladky Yes, that's what I plan to investigate. But looks like this can only be fixed for 3.x, not 2.x; not due to Ion backend but Jackson's way of initializing things. So I'll probably verify it can be made to work with 3.0 with "default pretty printer". In future could def also expose Ion-java's own configurability for PP, if there is interest.

But I wish I understood the deal with IonSystem.newTextWriter() vs IonTextWriterBuilder... I assume disconnect in API is due to historical reasons.

cowtowncoder commented 3 years ago

Easy enough with 3.x since information is available at the point where IonWriter is constructed. Problem with 2.x is that JsonGenerator.setPrettyPrinter() is called after construction and that is too late wrt IonWriter creation.

Also noticing use of CloseSafeUTF8Writer: removed from 3.0 (should not be needed), but now curious as to original problem. May see if I can reproduce where this might have been problematic.