perwendel / spark

A simple expressive web framework for java. Spark has a kotlin DSL https://github.com/perwendel/spark-kotlin
Apache License 2.0
9.64k stars 1.56k forks source link

Calling response.body(String) overrides any object the Route returns #1044

Closed steveperkins closed 5 years ago

steveperkins commented 5 years ago

Like the poster in #385 I'm finding some quirkiness in the way the response body is set.

It seems that the String provided to response.body(String) is always used as the response body if an object is returned from a Route.handle(Request, Response) call. If the route sets response.body(String) and also returns a String, the returned String is what is returned to the client.

For example, this code always returns "This is what will actually be returned at the end of the chain" instead of a toString() of SomeObjectResponse object:

public class Server {

    public static void main(String[] args) {
        new Server();
    }

    protected BugReport() {
        Spark.get("/version", (request, response) -> {
            response.body("This is what will actually be returned at the end of the chain");
            return new SomeObjectResponse("v200.34");
        });
    }

    class SomeObjectResponse {
        private String version;

        public SomeObjectResponse(String version) {
            this.version = version;
        }

        public String getVersion() {
            return version;
        }
    }
}

But if we change it to return two Strings:

import spark.Spark;

public class Server {

    public static void main(String[] args) {
        new Server();
    }

    protected BugReport() {
        Spark.get("/version", (request, response) -> {
            response.body("This is what will actually be returned at the end of the chain");
            return "This is the new champion of the body";
        });
    }

    class SomeObjectResponse {
        private String version;

        public SomeObjectResponse(String version) {
            this.version = version;
        }

        public String getVersion() {
            return version;
        }
    }
}

then "This is the new champion of the body" is what is ultimately returned to the client.

I'm trying to work around the issue where the response body isn't provided to any of the after filters and ran into this problem by accident. It seems to be caused by spark.http.matching.Routes lines 70-80:

...
        if (content instanceof String) {
            String contentStr = (String) content;

            if (!contentStr.equals("")) {
                context.responseWrapper().body(contentStr);
            }
        }
    }
}

context.body().set(content);

To my untrained eye it seems like context.responseWrapper() should retain a reference to the object body even it it's not a String.

My use case is where I'm trying to create a filter that will transform object responses to JSON so I don't have to do the transformation in every Route.

tipsy commented 5 years ago

Hi @steveperkins. The whole return value vs body(value) setup is a bit messy, but there's not much that can be done without breaking backwards compatibility. I guarantee some people are relying on the odd behaviors of this (we did at my previous company). The best advice I can give is to learn how it works (you seem to have mapped it out) and be consistent in how you write your application.

steveperkins commented 5 years ago

@tipsy that's understandable, it would be bad news to break older code. Any suggestions for getting the response body in an after filter to be non-null?

tipsy commented 5 years ago

In one of my older projects I had a setBody(req, res, "Body") method which would set the body, as well as store the body value as a request-attribute. You would do getBody(req, res) to retrieve it. I would not recommend it at all, but it's an option if you can't find a better way (I couldn't at the time).

steveperkins commented 5 years ago

@tipsy thanks for the tip. I'll keep searching for a reasonable way to manage this issue. I've started a new project and I love Spark Java's simplicity, but I don't want to institute a series of hacks just to get it to function as intended.

tipsy commented 5 years ago

If you think of something smart, please let us know.