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

Honor overriden writeObject of JsonGenerator passed to setJsonGeneratorDecorator #968

Conversation

rdesgroppes
Copy link
Contributor

@rdesgroppes rdesgroppes commented Jun 7, 2023

The order in which JsonGeneratorDelegates are chained currently leads to disregard the writeObject method of the JsonGenerator passed to setJsonGeneratorDecorator.

The internal SimpleObjectJsonGeneratorDelegate indeed intercepts writeObject for quite a number of concrete types:

public class SimpleObjectJsonGeneratorDelegate extends JsonGeneratorDelegate {
public SimpleObjectJsonGeneratorDelegate(JsonGenerator delegate) {
super(delegate, false);
}
@Override
public void writeObject(Object value) throws IOException {
if (value == null) {
writeNull();
return;
}
if (value instanceof String) {
writeString((String) value);
return;
}
if (value instanceof Number) {
Number n = (Number) value;
if (n instanceof Integer) {
writeNumber(n.intValue());
return;
}
if (n instanceof Long) {
writeNumber(n.longValue());

// etc.

The present change therefore consists in moving any user-defined JsonGenerator to the top level of the delegation chain, leaving users the power (and therefore the responsibility) to adjust the output to their needs.

@rdesgroppes rdesgroppes force-pushed the honor-user-overriden-json-generator-write-object branch 3 times, most recently from 0c028d2 to fde90d9 Compare June 8, 2023 06:17
@rdesgroppes rdesgroppes changed the title setJsonGeneratorDecorator: honor overriden writeObject Honor overriden writeObject of JsonGenerator passed to setJsonGeneratorDecorator Jun 9, 2023
…neratorDecorator`

The order in which `JsonGeneratorDelegate`s are chained currently leads
to disregard the `writeObject` method of the `JsonGenerator` passed to
`setJsonGeneratorDecorator`.

The internal `SimpleObjectJsonGeneratorDelegate` indeed intercepts
`writeObject` for quite a number of concrete types:
https://github.com/logfellow/logstash-logback-encoder/blob/933a0f51c1bfefc838adee9982ad3537a08a39bd/src/main/java/net/logstash/logback/util/SimpleObjectJsonGeneratorDelegate.java#L38-L60

The present change therefore consists in moving any user-defined
`JsonGenerator` to the top level of the delegation chain, leaving users
the power (and therefore the responsibility) to adjust the output to
their needs.
@rdesgroppes rdesgroppes force-pushed the honor-user-overriden-json-generator-write-object branch from fde90d9 to c82d742 Compare June 9, 2023 13:59
@rdesgroppes
Copy link
Contributor Author

Hi @brenuart, may I kindly request a review? Merci !

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #968 (c82d742) into main (933a0f5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main     #968   +/-   ##
=========================================
  Coverage     71.92%   71.92%           
  Complexity     1356     1356           
=========================================
  Files           173      173           
  Lines          5011     5011           
  Branches        519      519           
=========================================
  Hits           3604     3604           
  Misses         1153     1153           
  Partials        254      254           
Impacted Files Coverage Δ
...back/composite/AbstractCompositeJsonFormatter.java 80.55% <100.00%> (ø)

@philsttr philsttr merged commit 9823bdf into logfellow:main Jun 17, 2023
@rdesgroppes rdesgroppes deleted the honor-user-overriden-json-generator-write-object branch June 19, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants