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.63k stars 1.56k forks source link

Using after filter modifies response status. #1122

Open aur8l opened 5 years ago

aur8l commented 5 years ago

Hi,

This seems to be a bug. When applying an after filter to a path group, if the route doesn't exist, the status of the response is changed systematically to 200. See below for reproducible example.

import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import org.slf4j.LoggerFactory;
import spark.Request;
import spark.Response;
import spark.Route;
import spark.servlet.SparkApplication;
import static spark.Spark.*;

public class Server implements SparkApplication {

    private static org.slf4j.Logger LOGGER = LoggerFactory.getLogger(Server.class);

    public static void main(String[] args) {

        new Server().init();
    }

    @Override
    public void init() {

        before("*", (request, response) -> {
            LOGGER.info("Request: " + request.url());
        });

        after((request, response) -> {
            response.type("application/json");
            response.header("Content-Encoding", "gzip");

        });

        /**
         * Let's declare routes
         */
        get("/ping", Server.ping);

        path("/api", () -> {
            //add query information to response object
            after("/*", (Request request, Response response) -> {
                Gson gson = new GsonBuilder().excludeFieldsWithoutExposeAnnotation().create();

                JsonObject body;
                if (response.body() != null) {
                    body = new JsonParser().parse(response.body()).getAsJsonObject();
                } else {
                    body = new JsonObject();
                }

                response.body(body.toString());
            });

            get("/person", Server.ping);
        });

    public static Route ping = (Request req, Response res) -> {
        res.status(200);
        return "Alive.";
    };
}

A call to /api/whatevs will not return 404, but 200. If I remove the filter, the it does return 404.

mlitcher commented 5 years ago

The logic in the MatcherFilter class only looks at whether the body is not null in deciding to return a 404. It doesn't take into account if there was a match on the route. In the code you provided, the after filter modifies the body regardless of whether the route was found. If you were to eliminate the else logic, you'd probably get the result you're looking for.

All that being said, I wonder if it may be better for the framework to detect whether the route was actually handled, as opposed to simply looking at the result.