Netflix / dgs-framework

GraphQL for Java with Spring Boot made easy.
https://netflix.github.io/dgs
Apache License 2.0
3.03k stars 286 forks source link

bug: webRequest.getResponse() is null #1869

Closed sharu closed 3 months ago

sharu commented 3 months ago

Please read our contributor guide before creating an issue.

Expected behavior

When DGS Version: 8.5.3 spring-graphql: 1.2.6 graphql-dgs-spring-graphql-starter: 8.5.3

public DataFetcherResult<DataType> fetchDataType(
            DgsDataFetchingEnvironment env) {
DgsWebMvcRequestData requestData = (DgsWebMvcRequestData) env.getDgsContext().getRequestData();
ServletWebRequest webRequest = (ServletWebRequest) requestData.getWebRequest();
HttpServletResponse servletResponse = webRequest.getResponse();

servletResponse.addHeader("Cache-Control", "private,no-cache, no-store, max-age=0");

Actual behavior

servletResponse is null and hence throwing NPE. This is not happening with graphql-dgs-spring-boot-starter

Steps to reproduce

servletResponse should not be null

srinivasankavitha commented 3 months ago

Hi @sharu - Thanks for trying out the new spring-graphql integration and for the feedback. I am able to reproduce your issue. However, the intent with making the request available here was mostly for access to request itself and related headers, not necessarily the response. So while this may have been working inadvertently, this wasn't the primary use case.

paulbakker commented 3 months ago

Unfortunately, we also don't have any control over this - the WebRequest instance is created by Spring. Depending on how it's created exactly in Spring it might or might not have a response object wrapped. So this happens in code outside the DGS framework. The difference in behavior probably results from the fact that previously, we used traditional WebMVC, while now it uses router functions.

I think your specific use case of setting a header would be better done in a filter?

sharu commented 3 months ago

Hi,

We were following this documentation for older versions https://netflix.github.io/dgs/datafetching/

@DgsQuery
@DgsMutation
public String updateCookie(@InputArgument String value, DgsDataFetchingEnvironment dfe) {
    DgsWebMvcRequestData requestData = (DgsWebMvcRequestData) dfe.getDgsContext().getRequestData();
    ServletWebRequest webRequest = (ServletWebRequest) requestData.getWebRequest();
    javax.servlet.http.Cookie cookie = new javax.servlet.http.Cookie("mydgscookie", value);
    webRequest.getResponse().addCookie(cookie);

    return value;
}

We have a requirement to send different cache control headers for each query. Some of the queries are cacheable and some are not. For example, some of feed won't get refreshed for a day, so we can cache it for longer duration. Personalised feeds should not be cached.

All generic header setting are done using

@Override
    public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws
            IOException, ServletException {
           }
paulbakker commented 3 months ago

@sharu I was wrong about it being set by Spring, we were actually dropping it. I've got a PR with a fix: #1870

sharu commented 3 months ago

@sharu I was wrong about it being set by Spring, we were actually dropping it. I've got a PR with a fix: #1870

Thanks for the quick fix. Looking forward to using this :)