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

DataBinder using default auto grow collection limit #392

Closed
j-kitch opened this issue May 20, 2022 · 7 comments
Closed

DataBinder using default auto grow collection limit #392

j-kitch opened this issue May 20, 2022 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@j-kitch
Copy link

j-kitch commented May 20, 2022

Hi,

My team has run into an issue with sending GraphQL requests with lists with greater than 256 elements over HTTP. This is occurring when the list is either directly in the query, or sent as a variable.

I think from my investigation that this is due to using the default DataBinder limit DataBinder.DEFAULT_AUTO_GROW_COLLECTION_LIMIT as we simply construct a normal DataBinder instance here:

DataBinder binder = new DataBinder(null, argumentName != null ? argumentName : "arguments");

I would have expected a more generous default, the ability to configure the property, or inject a different instance.

This is my first issue I've reported here so apologies if I've made any mistakes or need to change the issue - thanks!

@j-kitch j-kitch changed the title Data Binder using default auto grow collection limit DataBinder using default auto grow collection limit May 20, 2022
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 20, 2022
@bclozel bclozel self-assigned this Jun 9, 2022
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 9, 2022
@bclozel bclozel added this to the 1.0 Backlog milestone Jun 9, 2022
@bclozel
Copy link
Member

bclozel commented Jun 9, 2022

Thanks for raising this issue!
While we could reuse the ConfigurableWebBindingInitializer from the Spring web support, this is very much web specific. So we'll probably provide a way to consume the DataBinder and configure it on the AnnotatedControllerConfigurer.

On the Spring Boot side, we might create a new configuration property to make things easier to configure.
We should probably also consider setting a higher default, GraphQL being heavily data oriented. @j-kitch do you have a suggestion for a higher, sensible default for this value?

@j-kitch
Copy link
Author

j-kitch commented Jun 16, 2022

Thanks for getting back on this @bclozel

I agree, staying decoupled from the web specific support and allowing consumption of the DataBinder to be configured in AnnotatedControllerConfigurer sounds like the right choice.

For a sensible default - 512 or 1024 seem large enough without allowing developers to shoot themselves in the foot.

@rstoyanchev rstoyanchev modified the milestones: 1.0 Backlog, 1.0.1 Jun 17, 2022
@CH-EricLundberg
Copy link

CH-EricLundberg commented Dec 5, 2022

How do I actually use this? I don't see anything in the documentation. I've tried setting the dataBinder in various ways to get above 1024 limit but nothing seems to stick. :/

@rstoyanchev
Copy link
Contributor

You can set a Consumer<DataBinder> on AnnotatedControllerConfigurer. Note that this applies only to 1.0.x only. For 1.1, with #516, we no longer use DataBinder and all the logic is in GraphQlArgumentBinder itself.

@CH-EricLundberg
Copy link

What is the proper way to get access to the AnnotatedControllerConfigurer to do that? I haven't had to use that before and what I've tried thus far hasn't worked. Thanks for the response, appreciate it.

@CH-EricLundberg
Copy link

So I tried:

@Component
class MyAnnotatedControllerConfigurer {

    public MyAnnotatedControllerConfigurer(AnnotatedControllerConfigurer annotatedControllerConfigurer) {
        annotatedControllerConfigurer.setDataBinderInitializer(dataBinder -> dataBinder.setAutoGrowCollectionLimit(20000));
        }
}

which is called, but does not seem to affect the limit. I.e., same error with more than 1024 items.

I did end up working around the issue for better/worse.

My mutation input object is along the lines of:

@Data
@AllArgsConstructor
@NoArgsConstructor
public class MyInput {

    //various fields
    private List<Long> ids;
}

if I remove the @NoArgsConstructor it seems the use the all args constructor and skips the limit checking all together, which will work for us. Unclear why the setting of the dataBinder above wasn't doing the trick, I'm assuming I was doing the databinding setAutoGrow wrong.

@bclozel
Copy link
Member

bclozel commented Dec 9, 2022

@CH-EricLundberg

In a Spring Boot application, you could replace the bean completely by declaring it in your configuration:

@Bean
public AnnotatedControllerConfigurer annotatedControllerConfigurer() {
  AnnotatedControllerConfigurer configurer = new AnnotatedControllerConfigurer();
  configurer.setDataBinderInitializer(dataBinder -> dataBinder.setAutoGrowCollectionLimit(20000));
  return configurer;
}

I believe that your first attempt was trying to post process an existing bean. You can do that with:

public class AnnotatedControllerConfigurerPostProcessor implements BeanPostProcessor {

   @Override
   public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
   	if (bean instanceof AnnotatedControllerConfigurer configurer) {
   		configurer.setDataBinderInitializer(dataBinder -> dataBinder.setAutoGrowCollectionLimit(20000));
   	}
   	return bean;
   }
}

and then declare that AnnotatedControllerConfigurerPostProcessor as a bean in your application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants