spring-graphql
spring-graphql copied to clipboard
spring-projects
Add http multipart support
This PR adds MultipartFile support for WebMVC and FilePart for WebFlux on server side, HttpGraphQlClient, HttpGraphQlTester support on client side. https://github.com/spring-projects/spring-graphql/issues/69
Test project https://github.com/nkonev/graphql.demo/tree/file-test-harness
@rstoyanchev Please take a look when you get a chance.
P. S. About formatting: I tried to fix it by installing spring-javaformat 034 jar (as described here https://github.com/spring-io/spring-javaformat), but it reports issues of files out of scope my PR. Can you please apply your configured formatter ?
P. S. S. The next step is to add handlers and coercing(optionally) to Spring Boot.
Sorry we didn't have time to have a deeper look. This is on our radar and we'll let you know once the entire team is back. Thanks!
I can rebase it onto main branch (future 1.1.x).
Any fixes, any explanations, let me know if they are needed.
For full transparency, we've discussed this today and we've scheduled this issue (and #69) to the 1.1 Backlog. We don't think we'll get to it in time for 1.1.0, but this is definitely on our radar. Our existing 1.1 milestones are already quite full and we want to make sure we get this right.
We'd like to spend more time experimenting with this and thinking about ways to address CSRF issues in general. We might evolve this PR, or selectively pick things up from it. In any case, your contribution will be recognized as it should.
Is there any update/eta on this? I see the issue itself is on the 1.2 backlog now that doesn't specify any due date or something.
we have same file upload via graphQL requirement.... Any update on @nkonev proposed fix? @bclozel
@nkonev apologies for the delay in getting back to you.
We've discussed this on the team, and think that file uploads embedded in GraphQL requests adds unnecessary complexity vs using an HTTP endpoint to handle multipart requests. A @MutationMapping method for file uploads would look very similar to a @PostMapping method for the same, and would have the same implementation, but the latter is based on existing standards and support.
Looking at Apollo Server File Upload Best Practices it seems they've evolved their approach, also moving away from the file uploads spec. The article recommends against using file uploads in strong terms, and suggests to separate handling of file uploads from the handling of GraphQL requests for a variety of reasons including security and performance.
Noting what Apollo has done from 3.0 to keep the graphql-upload package external, one alternative would be to explore the option of keeping this as an externally maintained extension. I don't know how easy that would be in terms of the available extensions points, but it's an option to explore.
As per my last comment, we are not going to have multipart uploads support built in, but I want to make sure there is enough flexibility so it can exist independently.
Looking at the PR as a model, on the server side there can be a separate multipart handler that creates a request and passes it down the chain. On the client side, it seems to come down to a slightly customized WebClient call, which would be straight forward without involving GraphQlClient, and the same for testing with WebTestClient. If there is anything specific we could do there, please let us know.
Overall, the code could become simpler if built more explicitly for HTTP, and separate from the main API vs trying to fit into a transport agnostic base. It also connects back to the mismatch between GraphQL being transport agnostic vs multipart uploads being an HTTP only mechanism, and if you must implement the spec, might as well model it more explicitly along those lines.
there can be a separate multipart handler that creates a request
@rstoyanchev
About server. To make it possible to write Multipart GraphQlHttpHandler we need to have access to WebGraphQlRequest's query, operationName, variables. Right now they aren't accessible - they are extracted in the constructor
public WebGraphQlRequest(...
super(getKey("query", body), getKey("operationName", body), getKey("variables", body),
getKey("extensions", body), id, locale);
Are you ok to add one more constructor in separate PR ? To make it simpler to write Multipart GraphQlHttpHandler (than here - see MultipartGraphQlRequest - it will gone if we add the that constructor)
public WebGraphQlRequest(
URI uri, HttpHeaders headers,
String query,
String operationName,
Map<String, Object> variables,
Map<String, Object> extensions,
String id, @Nullable Locale locale) {
super(query, operationName, variables, extensions, id, locale);
Assert.notNull(uri, "URI is required'");
Assert.notNull(headers, "HttpHeaders is required'");
this.uri = UriComponentsBuilder.fromUri(uri).build(true);
this.headers = headers;
}
About client. It's interesting - to just customize WebClient. Indeed, at first glance it looks attractive, because the only meaningful thing is
@Override
public Mono<GraphQlResponse> executeFileUpload(GraphQlRequest request) {
return this.webClient.post()
.contentType(MediaType.MULTIPART_FORM_DATA)
.accept(MediaType.APPLICATION_JSON, MediaType.APPLICATION_GRAPHQL)
.body(BodyInserters.fromMultipartData(convertRequestToMultipartData(request)))
.retrieve()
.bodyToMono(MAP_TYPE)
.map(ResponseMapGraphQlResponse::new);
}
Closing this for now as we don't intend to proceed as per my earlier https://github.com/spring-projects/spring-graphql/pull/430#issuecomment-1476186878. @nkonev, thanks for all the effort in any case!
Yes, please don't remove pr in order to let me or someone else make this functionality separately, plugin-like
There is no way to delete a pull request. Is that what you meant? It's just in "closed" instead of "open" state, but it's still possible to search and find.