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

Pseudo restore PathMappings.getMatch(String) for backwards compat reasons #8628

Closed
mark-christiaens opened this issue Sep 28, 2022 · 5 comments · Fixed by #8629
Closed

Pseudo restore PathMappings.getMatch(String) for backwards compat reasons #8628

mark-christiaens opened this issue Sep 28, 2022 · 5 comments · Fixed by #8629
Labels
Bug For general bugs on Jetty side

Comments

@mark-christiaens
Copy link

Jetty version(s)
10.0.10

Java version/vendor

Not relevant

OS type/version

Not relevant

Description

Commit 8de55150fe1889c2c748b821a4f76bce9cbf8f9a deprecated PathMappings#getMatch(String) and made it throw an UnsupportedOperationException.

    /** 
     * @deprecated use {@link #getMatched(String)} instead
     */
    @Deprecated
    public MappedResource<E> getMatch(String path)
    {   
        throw new UnsupportedOperationException("Use .getMatched(String) instead");                                                                                   
    }   

This is a non-backwards compatible change. Our code now throws exceptions. I suspect this should not be done in a minor release.

How to reproduce?

Not relevant

@mark-christiaens mark-christiaens added the Bug For general bugs on Jetty side label Sep 28, 2022
@joakime
Copy link
Contributor

joakime commented Sep 28, 2022

A bit surprised someone is using PathMapping directly like this.
It's not really exposed to outside use directly like that.
Typical usage is to just give either the ServletHolder/ServletHandler or WebSocketServer tree a PathSpec and the underlying implementation uses the PathMappings object to manage them.
How are you using it?

Anyway, the .getMatch(String path) was set to UnsupportedOperationException in commit 8de5515
I don't recall why.
I'll post a PR to restore this, simply by calling .getMatched(String) internally.

@joakime
Copy link
Contributor

joakime commented Sep 28, 2022

Ah, yes.

The API is designed now to return MatchedResource which returns the pathSpec, Resource, pathMap, and pathInfo on the hit.
The old MappedResource is still there, as the input to establish the mapping, but not returned in any .getMatch*() API.

The MappedResource doesn't have the pathMap and pathInfo bits.
The new MatchedResource does, and is an object used with in the dispatch in Servlet and WebSocket Server.
This skips another lookup/match for the Servlet Name, Servlet Path, and Path Info portions of the various specs.

A pseudo backward compatible fix is to have the old .getMatch(String) return a new MappedResource every time it is called, and skipping the information from MatchedResource.

A quick workaround would be ...

public class OldPathMappings<E> extends PathMappings<E>
{
    @Override
    public MappedResource<E> getMatch(String path)
    {
        MatchedResource<E> matchedPath = getMatched(path);
        return new MappedResource<>(matchedPath.getPathSpec(), matchedPath.getResource());
    }
}

This API .getMatch(String) is flagged as deprecated and is flagged for deletion. (in fact it's already been deleted in Jetty 12)

joakime added a commit that referenced this issue Sep 28, 2022
+ This returns a MappedResource, but not
  the stored instance, but a new instance
  of MappedResource every time.

+ Flagged deprecated APIs for removal
  as well

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime linked a pull request Sep 28, 2022 that will close this issue
@joakime
Copy link
Contributor

joakime commented Sep 28, 2022

A pseudo backward compat option has been opened as PR #8629

joakime added a commit that referenced this issue Oct 6, 2022
…g-getmatch

Issue #8628 - pseudo restore `PathMappings.getMatch(String)`
@margus2
Copy link

margus2 commented Oct 13, 2022

This issue affects Jetty 9.4.49 too.

@joakime
Copy link
Contributor

joakime commented Oct 13, 2022

@margus2 Jetty 9.4.x is at End of Community support.

@joakime joakime changed the title Non backwards compatible change in minor release Pseudo restore PathMappings.getMatch(String) for backwards compat reasons Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants