Skip to content

Resources handler POC#27

Merged
stevehu merged 3 commits intodevelopfrom
feature/resources-handler
Apr 29, 2018
Merged

Resources handler POC#27
stevehu merged 3 commits intodevelopfrom
feature/resources-handler

Conversation

@DSchrupert
Copy link
Copy Markdown
Member

Not ready for merge

Hi @stevehu ,

I just wanted to get a sense from you for this solution. In order to provide hybrid services the ability to serve static resources on the same server, i'm hoping this would get the job done.

Each hybrid service within the server could have multiple classes implement the RpcResourcePathsProvider interface. At startup, the hook will run through identically to how you search for classes annotated with ServiceHandler, and will append to a list of safe paths.

Any requests coming into the /resources/* path, are then compared to that safe list (undertow built in) to see if they are available to serve.

For example:
If your server jar had the following directories:

/com
/config
/view
   |-> index.html

And there was a class in the jar that implemented RpcResourcePathsProvider and returned ["view"] from the getSafeResourcePaths method. Then the index.html would be retrievable at the /resources/view/index.html path, however nothing in the /config or /com folders would be.

Does this seem like a viable solution to continue working on?

@DSchrupert DSchrupert requested a review from stevehu April 16, 2018 05:51
@stevehu
Copy link
Copy Markdown
Contributor

stevehu commented Apr 16, 2018

Great idea!!! I was thinking about this feature for a long time as it is very useful for API server to serve resources(Static content or Javascript SPA). Previously, I was thinking to create a generic handler in light-4j to do the job but never got a chance to do that. Here is an example I did long time ago with injection in path handler provider. I am not sure if this is a good idea as it is a little bit harder for the codegen. Let me take a look at your implementation and we can have a chat when you are available.

@DSchrupert
Copy link
Copy Markdown
Member Author

DSchrupert commented Apr 16, 2018

@stevehu i think your suggestion of a generic resource handler in light4j makes more sense then this change.

The way i understand the generic solution would be as follows (as an example with codegen-web):

  1. A new get handler/config to be added to light-4j. Lets say ResourcesHandler
  2. The resources.yml associated could have the following property:
resourceManagerProvider: com.networknt.codegen.CodegenResourceManagerProvider

com.networknt.codegen.CodegenResourceManagerProvider would implement some interface which could supply an implementation ofio.undertow.server.handlers.resource.ResourceManager.

  1. When a new instance of the ResourcesHandler is added to the routing rules:
@Override
public HttpHandler getHandler() {
    return Handlers.routing().add(Methods.GET, "/v1/resources", new ResourcesHandler())
}

The CodegenResourceManager would be instantiate.

  1. All requests to that path then would be handled by the ResourceHandler instantiated with the client ResourceManager.

Does this sound better? It certainly gives the client a lot of control (resource serving path, which resources are made available, and which type of ResourceManager they provide), while allowing it to be driven by the standard light-4j configuration.

I'm interested to hear your thoughts!

@stevehu
Copy link
Copy Markdown
Contributor

stevehu commented Apr 16, 2018

There are other things that need to be considered. Like how to handle the url rewrite for the SPA. How to serve static content like /images etc. That is why PredicatedHandlerParser is used in the following handler.

    public HttpHandler getHandler() {

        return Handlers.predicates(
                PredicatedHandlersParser.parse("not path-prefix('/images', '/assets', '/api') -> rewrite('/index.html')"
                        , WebServerHandlerProvider.class.getClassLoader()),
                new PathHandler(resource(new FileResourceManager(
                        new File(config.getBase()), config.getTransferMinSize())))
                        .addPrefixPath("/api/json", new JsonHandler(Config.getInstance().getMapper()))
                        .addPrefixPath("/api/text", new TextHandler())
        );
    }

@DSchrupert
Copy link
Copy Markdown
Member Author

@stevehu that's a great point! One that I certainly haven't yet covered in this pr, but would be easily integrated with the solution in my last comment.

@stevehu
Copy link
Copy Markdown
Contributor

stevehu commented Apr 16, 2018

Yes. We also need to test with single page application proxy as well. Most time, when using Webpack, a proxy will be started on the Nodejs and connect to backend Java API. So CORS handler needs to be enabled on the Java API.

@stevehu
Copy link
Copy Markdown
Contributor

stevehu commented Apr 22, 2018

@NicholasAzar I have reviewed your code in both light-hybrid-4j and light-4j. The idea is great with this level of injection. It is better than the example I provided as it has to rely on the light-codegen. This approach gives users most flexibility. By comparing with the current develop branch in light-hybrid-4j, I see some conflicts. The latest change in the develop branch is to make each handler path configurable, so it should be easy to merge from develop to resources-handler. Thanks a lot for the effort.

@DSchrupert
Copy link
Copy Markdown
Member Author

@stevehu sorry i couldn't get to this sooner.. conflicts should be resolved now. I hope it's ok to make the checks for configured paths a bit more concise.. i don't know if the double handler possibility of before was desired?

@stevehu stevehu merged commit caf7844 into develop Apr 29, 2018
@stevehu
Copy link
Copy Markdown
Contributor

stevehu commented Apr 30, 2018

@NicholasAzar I have merged this PR and also created a pull request for your changes in light-4j. Automatic build passed with all tests. Thanks a lot for your help.

@stevehu stevehu deleted the feature/resources-handler branch March 24, 2019 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants