Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetty 12 - Introduce PathMappingsHandler #8748

Merged
merged 9 commits into from
Oct 25, 2022

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Oct 20, 2022

New Handler allowing sub-handlers to be selected via a PathMappings configuration.

@joakime joakime requested review from gregw and sbordet October 20, 2022 15:27
@joakime joakime self-assigned this Oct 20, 2022
@joakime joakime requested a review from sbordet October 20, 2022 17:07
@sbordet sbordet self-requested a review October 20, 2022 17:10
@joakime joakime requested a review from sbordet October 20, 2022 17:13
@joakime
Copy link
Contributor Author

joakime commented Oct 20, 2022

If we are ok with the idea of this, I'll write up some better javadoc for that class. (add some better tests for contexts other than / too)


PathMappingsHandler pathMappingsHandler = new PathMappingsHandler();
pathMappingsHandler.setHandler(new SimpleHandler("WrapperFoo Hit"));
pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpExample Hit"));
Copy link
Contributor Author

@joakime joakime Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example 1 (of 2) that might be useful for FCGI (to PHP) Proxy.
This one relies on no-match behavior to fallback to the wrapper (which would be the ResourceHandler in the FCGI case)

pathMappingsHandler.addMapping(new ServletPathSpec("/"), new SimpleHandler("FakeResourceHandler Hit"));
pathMappingsHandler.addMapping(new ServletPathSpec("/index.html"), new SimpleHandler("FakeSpecificStaticHandler Hit"));
pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new SimpleHandler("PhpHandler Hit"));
contextHandler.setHandler(pathMappingsHandler);
Copy link
Contributor Author

@joakime joakime Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example 2 (of 2) that might be useful for FCGI (to PHP) Proxy.
This one relies on default servlet pathspec behavior to use the resource handler for non-php requests.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a bad idea, but needs some work.

Maybe there should be a common ancestor of this and ContextHandlerCollection that handles the whole Handler.Container conversation about descendants and bean management. Then some abstract methods can be used to make it either a ContextHandlerCollection or a PathMapHandlerCollection?

Maybe Handler.Collection is already that common ancestor?

@gregw
Copy link
Contributor

gregw commented Oct 20, 2022

See also the vaguely related #8676

@gregw
Copy link
Contributor

gregw commented Oct 20, 2022

Perhaps we should look at the feedback I've given in this PR and put it into a "How to write a Handler" guide documentation? Calling all tech writers!!!!

@gregw
Copy link
Contributor

gregw commented Oct 24, 2022

+1 for extending AbstractContainer
How about Handler.Sequence rather than Handler.Collection or Handler.List ?

@joakime
Copy link
Contributor Author

joakime commented Oct 24, 2022

Not a fan of Sequence here.

Mainly because the word "Sequence" here has meaning that the PathMappingsHandler isn't doing.
Also the next JDK is supposed to actually implement the "Sequenced Collections" JEP.

https://openjdk.org/jeps/431

@gregw
Copy link
Contributor

gregw commented Oct 24, 2022

Not a fan of Sequence here.

Mainly because the word "Sequence" here has meaning that the PathMappingsHandler isn't doing. Also the next JDK is supposed to actually implement the "Sequenced Collections" JEP.

But PathMappingsHandler would not be a Handler.Sequence. The suggestion is for it to extend Handler.AbstractContainer.
The existing Handler.Collection would separately be renamed to Handler.Sequence, because it does have sequential behavior. We know a JVM Sequence is coming, but it will be at least initially less common than List and Collection. But open to other names that totally avoid clashing with a JVM class.... but I can't think of any good ones. Maybe "Serial"?

@joakime
Copy link
Contributor Author

joakime commented Oct 24, 2022

Extending from Handler.AbstractContainer doesn't work, as that would require implementing the following methods (that have no meaning under a PathMappingsHandler)

public void addHandler(Handler handler);
public void addHandler(Supplier<Handler> supplier);
public List<Handler> getHandlers();
public List<Handler> getDescendants();
public <T extends Container> T getContainer(Handler handler, Class<T> type);

@joakime joakime requested review from gregw and sbordet October 24, 2022 15:22
@joakime
Copy link
Contributor Author

joakime commented Oct 24, 2022

Also, why does Handler.Wrapper extend Handler.AbstractContainer ?

@gregw
Copy link
Contributor

gregw commented Oct 24, 2022

Extending from Handler.AbstractContainer doesn't work, as that would require implementing the following methods (that have no meaning under a PathMappingsHandler)

public void addHandler(Handler handler);
public void addHandler(Supplier<Handler> supplier);
public List<Handler> getHandlers();
public List<Handler> getDescendants();
public <T extends Container> T getContainer(Handler handler, Class<T> type);

They can all have reasonable meanings for your handler:

  • addHandler could add a handler at the default mapping.... or it could be unsupported operation exception
  • ditto for the add supplier
  • getHandlers certainly has a meaning, it returns all the nested handlers - ie the maps value set
  • ditto for descendants, it returns all the descendant handlers of all contained handlers. Default impl will work fine
  • ditto for getContainer, default impl will work fine

Handler.Wrapper is a Container because it is a Handler that contains other Handlers. See how it extends Handler.Nested and has a meaningful interpretation of addHandler

See ContextHandlerCollection (although maybe that really should be ContextMapHandler extends AbstractContainer.. but that's another PR and some history... plus it does iterate if there is no obvious match). It's job is slightly easier for addHandler, as contexts carry their context paths with them.

Having all this stuff correct is important, so we can dump, find, manipulate handlers in a consistent way.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got one niggle, but will approve as I'm out of office for rest of today.
Niggle is make sure you remove the bean of any replaced handlers.

throw new IllegalStateException("addMapping loop detected: " + handler);
}

mappings.put(pathSpec, handler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this return an old handler that was replaced? If so, then that needs to be removed as a bean and probably returned from this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no replacement, the mappings are a Set or Index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened Issue #8764 to address this later

@joakime joakime merged commit 8eb10b2 into jetty-12.0.x Oct 25, 2022
@joakime joakime deleted the fix/jetty-12-new-pathmapping-handler branch October 25, 2022 15:04
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