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

Allow Jackson StreamReadConstraints to be configured #34709

Open
pjfanning opened this issue Mar 21, 2023 · 25 comments
Open

Allow Jackson StreamReadConstraints to be configured #34709

pjfanning opened this issue Mar 21, 2023 · 25 comments
Labels
status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Milestone

Comments

@pjfanning
Copy link

Jackson 2.15 (v2.15.0-rc1 is out) has a default set of limits that it applies to inputs. Input Files that breach those limits will cause parse exceptions.

spring-boot will need to consider if those limits are ok or if you need to provide a way to configure the Jackson StreamReadConstraints.

https://javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.15.0-rc1/com/fasterxml/jackson/core/StreamReadConstraints.html

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 21, 2023
@wilkinsona
Copy link
Member

wilkinsona commented Mar 22, 2023

Thanks for raising this, @pjfanning.

At the ObjectMapper level, which is where Spring Boot operates, I can't quite see how to configure the constraints. As far as I can tell, they have to be configured on a JsonFactoryBuilder that's used to create a JsonFactory which is used in turn to create the ObjectMapper. That JsonFactory should be a MappingJsonFactory which, ideally, should be created with an ObjectMapper. I can't see how to create an ObjectMapper with a custom JsonFactory that itself has been created with that same ObjectMapper.

@cowtowncoder if you have a moment, could you please point us in the right direction here?

@pjfanning
Copy link
Author

pjfanning commented Mar 22, 2023

Constructing a MappingJsonFactory from an existing ObjectMapper and then trying to override the StreamReadConstraints on the MappingJsonFactory - that is not currently allowed.

I'm not sure if this is something that jackson-databind should have but @cowtowncoder may see differently.

If there is no way for you to refactor your code, you could subclass jackson-databind's MappingJsonFactory and add a method to override the StreamReadConstraints.

@pjfanning
Copy link
Author

pjfanning commented Mar 22, 2023

@wilkinsona could you highlight some code that you are worried about? I looked at spring-boot and all I can find is lots of code that uses new ObjectMapper().

Instead of new ObjectMapper(), you might find it a good idea to have a common ObjectMapperFactory, so that it easier to replace all the new ObjectMapper() calls with something like ObjectMapperFactory.createObjectMapper().

private static JsonFactory JSON_FACTORY;

// add some code to create JSON_FACTORY with your preferred StreamReadConstraints settings

public static createObjectMapper() {
   return JsonMapper.builder(JSON_FACTORY).build();
}

@wilkinsona
Copy link
Member

My concern is a general one and doesn't really focus on any specific area of Spring Boot's code. If there's a mechanism in Jackson for configuring the constraints then based on our past experience with configuring Jackson I expect that we'll be able to use it without too much difficulty. It's a configuration mechanism that's similar to what Jackson offers in various other areas that I am looking for.

If there is one area that's likely to be of specific concern it is in and around org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration. This auto-configuration applies the user's spring.jackson.* application properties and makes extensive use of Spring Framework's Jackson2ObjectMapperBuilder to do so. If the constraints need to be configurable, some new spring.jackson.* properties would be the obvious way for users to do so. I don't yet see how we could apply those properties to either a new or an existing ObjectMapper instance.

@pjfanning
Copy link
Author

Jackson is moving towards making Object Mappers immutable. You may need to rethink the idea of reconfiguring at that level. Factories and Builders are where Jackson config happens or that's where things are moving.

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Mar 22, 2023
@cowtowncoder
Copy link

cowtowncoder commented Mar 22, 2023

Ok, while @pjfanning is correct in that we are moving to Builder-based configuration, there does need to be some support for old-style direct configuration for compatibility reasons. For new functionality this may not be needed but for StreamReadConstraints we do need something, I think. Jackson 3.x (only) will offer/require full immutability; before that there is just availability of Builders with only partial benefits.

I will file a jackson-core issue to make sure there is actually a way to directly set configuration via JsonFactory -- I do believe that it is impractical to expect frameworks to switch to Builder-style construction (and more importantly, exposing that to their users) mid-Jackson-2.x

EDIT: created FasterXML/jackson-core#962

Thank you @pjfanning for due diligency here: I think this is something that must be resolved for 2.15.0 final & is a good catch now (instead of after release).

@philwebb philwebb changed the title please do not upgrade to jackson 2.15 without considering the implications of the new StreamReadConstraints Please do not upgrade to jackson 2.15 without considering the implications of the new StreamReadConstraints Mar 22, 2023
@philwebb philwebb added type: enhancement A general enhancement status: blocked An issue that's blocked on an external project change and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Mar 22, 2023
@philwebb philwebb changed the title Please do not upgrade to jackson 2.15 without considering the implications of the new StreamReadConstraints Allow Jackson StreamReadConstraints to be configured Mar 22, 2023
@philwebb philwebb added this to the 3.1.x milestone Mar 22, 2023
@cowtowncoder
Copy link

@wilkinsona I did add the obvious 2.x, non-builder mechanism (see FasterXML/jackson-core#962), that is, JsonFactory.setStreamReadConstraints(). It will be in 2.15.0-rc2. Thank you for outlining this obvious (in hindsight, although I really should have seen it ahead of time too) gap.

As usual, help by Spring Boot team is much appreciated: as much as I want 2.15.0 to get done ASAP, I want to also avoid rushing and breaking existing usage by valued user communities.

@wilkinsona
Copy link
Member

Thanks very much, @cowtowncoder.

@wilkinsona
Copy link
Member

With the new setter, it'll be possible for someone to configure the constraints in a Spring Boot application without requiring any changes in Boot:

@Bean
Jackson2ObjectMapperBuilderCustomizer customStreamReadConstraints() {
	return (builder) -> builder.postConfigurer((objectMapper) -> objectMapper.getFactory()
		.setStreamReadConstraints(StreamReadConstraints.builder().maxNestingDepth(2000).build()));
}

This will ensure that any ObjectMapper created through the auto-configured Jackson2ObjectMapperBuilder will have a max nesting depth of 2000. We can still consider adding some configuration properties for the three constraints if tuning them is a common requirement.

@cowtowncoder
Copy link

Correct @wilkinsona, that's the idea.

@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.x Mar 24, 2023
@jamesdh
Copy link

jamesdh commented Jun 15, 2023

We've added a Jackson2ObjectMapperBuilderCustomizer to modify the max string length like @wilkinsona mentioned but we're not seeing this reflected in our tests that use an injected ObjectMapper. We've also tried using StreamReadConstraints.overrideDefaultStreamReadConstraints to force override at application startup, but again, no luck. Still hitting the 20,000,000 limit.

@Configuration
public class JacksonConfig {
    @Bean
    Jackson2ObjectMapperBuilderCustomizer customStreamReadConstraints() {
        return (builder) -> builder.postConfigurer((objectMapper) -> objectMapper.getFactory()
            .setStreamReadConstraints(StreamReadConstraints.builder().maxStringLength(100000000).build()));
    }
}
public static void main(String[] args) {
    StreamReadConstraints.overrideDefaultStreamReadConstraints(
        StreamReadConstraints.builder().maxStringLength(100000000).build()
    );
    SpringApplication.run(ApiApplication.class, args);
}

@jamesdh
Copy link

jamesdh commented Jun 15, 2023

The only way I could get this to work was to override the defaults in a static context, e.g.

@Configuration
public class JacksonConfig {
    static {
        StreamReadConstraints.overrideDefaultStreamReadConstraints(
            StreamReadConstraints.builder().maxStringLength(100000000).build()
        );
    }
}

@wilkinsona
Copy link
Member

wilkinsona commented Jun 15, 2023

@jamesdh I can't reproduce the behaviour you've described in either case. If overrideDefaultStreamReadConstraints is not working, it sounds like something else is configuring the constraints and preventing the overridden defaults from taking effect. Please follow up with a question on Stack Overflow that includes a minimal example that reproduces the problem.

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Jun 15, 2023
@jamesdh
Copy link

jamesdh commented Jun 15, 2023

@wilkinsona I got overrideDefaultStreamReadConstraints working but had to call it from a static context. I guess that implies some other bean/config in a 3rd party dependency could be at cause? We have nothing else in our code that touches the ObjectMapper config.

@wilkinsona
Copy link
Member

It's really a Jackson question, but as far as I know changes to the defaults should affect every com.fasterxml.jackson.core.JsonFactory that's created thereafter. As I said above, please follow up on Stack Overflow as this issue isn't that right place for this.

@cowtowncoder
Copy link

I think this issue should be closed as the original change was done.

And if there are follow-up issues, file issue in appropriate place(s).

@wilkinsona
Copy link
Member

I'd like to keep the issue open to see if there's sufficient interest in some spring.jackson.* configuration properties for configuring the constraints. In the meantime, using a customizer or an early call to StreamReadConstraints.overrideDefaultStreamReadConstraints remains the recommended approach.

@jjstreet
Copy link

We just ran into this situation. Received a constraints limit reached. It would be nice to be able to set this via properties as most of the configuration we do is via those properties.

@cowtowncoder
Copy link

cowtowncoder commented Jul 26, 2023

Jackson does not offer much any global configutation, by design -- it is typically embedded as a component so global configuration tends to break usage in unintended places.

So no support for property configuration will be added on Jackson side.

EDIT: please disregard, as per @wilkinsona comment I was confused about question; not related to Jackson side. Spring definitely has lots of relevant configurability.

@wilkinsona
Copy link
Member

@cowtowncoder, I assume @jjstreet was referring to Spring Boot properties. We already provide several for configuring an ObjectMapper instance and could add to them to provide property-based configuration for its stream read constraints.

@cowtowncoder
Copy link

@wilkinsona Thank you for pointing this out -- yeah I should pay attention to where issue exists, not just look at title & update :)

@gyula-lakatos
Copy link

@cowtowncoder, I assume @jjstreet was referring to Spring Boot properties. We already provide several for configuring an ObjectMapper instance and could add to them to provide property-based configuration for its stream read constraints.

It would be quite useful. I keep hitting certain limits every now and then. Copying the same configuration class to every microservice is quite suboptimal. Releasing a jar that has the ability to enable the configuration via properties and including it as a dependency in every app is even more problematic.

@jjstreet
Copy link

jjstreet commented Aug 4, 2023

I will say that we were able to route around the stream limit constraints by using the suggested customizer class.

Where would such a property exist? spring.jackson.deserialization.*?

@wilkinsona
Copy link
Member

I'm not sure but it couldn't be a spring.jackson.deserialization property as they're a Map<DeserializationFeature, Boolean>. We'll have to give it some thought.

@wilkinsona wilkinsona added the status: pending-design-work Needs design work before any code can be developed label Aug 4, 2023
@smarbl-AmitRohra
Copy link

smarbl-AmitRohra commented Feb 21, 2024

The only way I could get this to work was to override the defaults in a static context, e.g.

@Configuration
public class JacksonConfig {
    static {
        StreamReadConstraints.overrideDefaultStreamReadConstraints(
            StreamReadConstraints.builder().maxStringLength(100000000).build()
        );
    }
}

This works to allow larger string/json as input

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

9 participants