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

Improve options to customize of ConversionService used for GraphQL argument binding #271

Closed
MothF opened this issue Jan 26, 2022 · 4 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@MothF
Copy link

MothF commented Jan 26, 2022

Spring Boot version: 2.7.0-M1
Spring GraphQL version: 1.0.0-M5

Hello. I faced a problem with arguments resolution when passing them to GraphQL controller methods

I have a declared method accepting some input DTO which may have field of one of the types: OffsetDateTime, ZonedDateTime, LocalDateTime. In terms of GraphQl variables it’s a date-formatted string.
There is also related graphql.schema.Coercing responsible for deserialization into java.time.Instant. So in general I want to convert java.time.Instant into correct date representation.

There are several cases where I need customization:

  1. Controller plain argument mapping (ex. OffsetDateTime/ZonedDateTime it depends)
  2. Controller DTO argument mapping (ex. TestInput)

In the first case I need a way to customize ConversionService but without overriding AnnotatedControllerConfigurer bean because it may have unpredictable consequences in future (as I'm developing custom library it would be impossible to maintain). For example it would be okay to call conversion service setter in some configuration PostConstruct method, but as for now it won’t have any effect because of hardcoded GraphQlArgumentInitializer at afterPropertySet method

In the second case there are several problems:

  1. If I want to use no-args-constructor and setters deserialization GraphQlArgumentInitializer will use DataBinder. In such case if something is wrong with mapping into DTO DataBinder doesn’t throw any exception. In my opinion if DataBinder contains some errors they should be re-thrown as it’s done in all-args-constructor case.
  2. There should be a way for DataBinder customization. As DataBinder is hardcoded I’m not able to do this. Check this line

So I think that ConversionService should be passed to DataBinder (something like this dataBinder.setConversionService(this.typeConverter.getConversionService());) and there should be an ability to customize ConversionService

If there is a workaround or my case is inapplicable I'd like to know about it. Also please check project with issues reproduction https://github.com/MothF/spring-graphql

Thank you in advance.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 26, 2022
@rstoyanchev
Copy link
Contributor

Indeed currently the ConversionService is created in the Boot starter without any option to customize. Not sure if we want to create a dedicated instance for GraphQL, or re-use the ones from Spring MVC and WebFlux. What do you think @bclozel?

On the side of AnnotatedControllerConfigurer we could defer the initialization by implementing SmartInitializingSingleton instead. That would allow more time to set the ConversionService.

As for setting the ConversionService on DataBinder that seems reasonable.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 1, 2022
@rstoyanchev rstoyanchev added this to the 1.0.0-M6 milestone Feb 1, 2022
@rstoyanchev rstoyanchev self-assigned this Feb 3, 2022
@rstoyanchev
Copy link
Contributor

I've made the ConversionService a fixed, internally managed instance within AnnotatedControllerConfigurer that you can customize via FormatterRegistrar callbacks so you should be able to add one of those. It's also now passed on to the DataBindiner instance. Please, give this a try.

@MothF
Copy link
Author

MothF commented Feb 3, 2022

Thanks! I'll try it as soon as I can.
I have one more question about DataBinder, if you don't mind me asking. Is it suitable to throw an exception out of it? If I'm not mistaken DataBinder collects all problems during binding and stores them internally.
As an example Long overflow: in such case (when Long is DTO field) I'll receive null instead of exception. But with plain method argument everything is fine and exception is thrown

I've made the ConversionService a fixed, internally managed instance within AnnotatedControllerConfigurer that you can customize via FormatterRegistrar callbacks so you should be able to add one of those. It's also now passed on to the DataBindiner instance. Please, give this a try.

@rstoyanchev
Copy link
Contributor

Good point about DataBinder errors. I've created #280 to address that.

@rstoyanchev rstoyanchev changed the title DataBinder customization and exception handling Improve options to customize of ConversionService used for GraphQL argument binding Mar 22, 2022
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

3 participants