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

Add FindAll to Routes>Service>Spark>RoutesFindAllTest implements #1006 #1097

Closed johnnybigoode-zz closed 5 years ago

johnnybigoode-zz commented 5 years ago

So, from what I understood. The class Spark starts the whole thing, but you can create additional Sparks with the static method spark.Service.ignite

Which literally gives us a Service object. The service class seems to have a field that holds all the routes called routes.

Exposing that information would be the way to go.

In the Routes class, there are no methods for returning the whole route list, so that's where I started.

Using a similar structure to the method findMultiple in ln 124. I noticed that we could use the method findTargetsForRequestedRoute to return all, but it depends on RounteEntry.matches method that states:

ln 47:

        if (
                (httpMethod == HttpMethod.before || httpMethod == HttpMethod.after || httpMethod == HttpMethod.afterafter)
                && (this.httpMethod == httpMethod)
                && this.path.equals(SparkUtils.ALL_PATHS)) {
            // Is filter and matches all
            return true;
        }

It was expected to have an option to return all. But it's not possible to find an httpMethod that returns ALL options. Maybe a better implementation would be creating a SparkUtils.ALL_METHODS and using something on those lines to create a better functionality.

In our case, this is not necessary. If getting all routes means exposing the routes available to the dev, is it unthinkable to find reason to make this method internal. Since the only thing available to change routes seems to be Routes.clear(); and it is only used by Service.initiateStop(), it's safe to say that routes are not mutable, so a findAll() just needs to return a copy of all routes available to the running service.

tldr:

I'm adding a Routes.findAll() method. Takes no arguments and returns a new List<RouteMatch> with all that's available. Since there's not http method, path or accepted type to add to the RouteMatch, I'm just adding all the routeEntry information to the results.

`matchSet.add(new RouteMatch(routeEntry.target, routeEntry.path, routeEntry.path, routeEntry.acceptedType));`

The Routes is used within Service, so I added a findAll to Routes and a findAllRoutesto Service and a static findAllRoutesto Spark.

New test for these features: RoutesFindAllTest.

buckelieg commented 5 years ago

Wow. Great! Just one thing to add - could the rersults return collection of RouteEntry objects which do contain HTTP method info: In the Service.class:

Collection<RouteEntry> routes();

If we map different HTTP methods to the same URL we could not set up the things correctly. Thank a lot!

johnnybigoode-zz commented 5 years ago

If we map different HTTP methods to the same URL we could not set up the things correctly.

While I understand your concern and your use case, I'm not sure how to go from here. From what I checked from the code, all RouteMatch situations require to pass a HttpMethod and it's not something I predicted.

Gonna reviews this and I'll get in touch.