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

Call relative and canonical path support #7839

Merged
merged 9 commits into from Oct 20, 2017

Conversation

Projects
None yet
5 participants
@seglo
Copy link
Contributor

commented Sep 17, 2017

Pull Request Checklist

  • Have you read How to write the perfect pull request?
  • Have you read through the contributor guidelines?
  • Have you signed the Lightbend CLA?
  • Have you [squashed your commits]? (Optional, but makes merge messages nicer.)
  • Have you added copyright headers to new files?
  • Have you checked that both Scala and Java APIs are updated?
  • Have you updated the documentation for both Scala and Java sections?
    • I updated Java and Scala docs. Should this feature be added to an existing documentation page, a new page, or are the code docs sufficient?
    • EDIT: Docs added.
  • Have you added tests for any changed functionality?

Helpful things

Fixes

n/a

Purpose

Add a feature to Call to generate a relative route given a RequestHeader. Using relative routes makes it easier to develop standalone Play! apps that can be hosted behind proxy's or other HTTP gateways regardless of their prefix route. Several examples include:

  • Hosting Play! apps behind a webserver proxy like DC/OS's adminrouter. I ran into this issue when trying to get Kafka Manager to work behind the DC/OS admin router.
  • When dynamically rendering stylesheets, you need asset links to be relative because they may end up getting served from different URLs by a CDN.

Background Context

Due to several issues[1] with java.nio.file.Path.relativize(Path other) I decided to roll my own implementation to create a canonical and relative path. I ported Python's posixpath implementation with a few minor additions to support routes that end without a trailing /.

The implementation is in Java and self-contained within the play.mvc.Call type. Two new public methods are available:

  • String relative(Http.RequestHeader requestHeader)
  • String canonical()

The relative implementation is based on relpath (from posixpath.py) and commonprefix (from genericpath.py) in the Python 2.7 os.path package. To canonicalize the URL first I ported the normpath implementation (from posixpath.py) also in the os.path package.

I also included a suite of tests to support variations of paths to relativize and canonicalize.

References

Feature proposal on the play-framework-dev list: https://groups.google.com/forum/#!topic/play-framework-dev/SCXWrbk1k-s

[1] https://bugs.openjdk.java.net/browse/JDK-6925169, https://stackoverflow.com/a/37749932/895309

@seglo seglo force-pushed the seglo:relative-routes branch from 05d7d4b to 7a6d134 Sep 18, 2017

@seglo seglo force-pushed the seglo:relative-routes branch from 7a6d134 to 9aee6c7 Sep 18, 2017

@gmethvin
Copy link
Member

left a comment

I see you mentioned you looked at java.nio.file.Path but it would be good to document what was missing that caused us to need a completely custom implementation here.

* // == "../url"
* }}}
*/
def relative()(implicit request: RequestHeader): String = this.relative(request.path)

This comment has been minimized.

Copy link
@gmethvin

gmethvin Sep 18, 2017

Member

I don't think we need the () here.

return relative(requestHeader.path());
}

protected String relative(String startPath) {

This comment has been minimized.

Copy link
@gmethvin

gmethvin Sep 18, 2017

Member

Maybe this should be a public method public String relativeTo(String path).

This comment has been minimized.

Copy link
@gmethvin

gmethvin Sep 18, 2017

Member

Also this seems like a lot of the logic here could be extracted into a utility method for use elsewhere. Something like PathUtils.relativize.

This comment has been minimized.

Copy link
@marcospereira

marcospereira Sep 18, 2017

Member

When changing this to a public method, it could be good to also improve documentation (with examples) for this method.

@@ -133,6 +139,111 @@ public String webSocketURL(boolean secure, String host) {
return "ws" + (secure ? "s" : "") + "://" + host + this.url();
}

private static String CURDIR = ".";

This comment has been minimized.

Copy link
@marcospereira

marcospereira Sep 18, 2017

Member

Can we have the full names here? CURRENT_DIR, SEPARATOR, etc.

String trailingSep = url.endsWith(SEP) ? SEP : "";

return prefixSep + canonical.stream().collect(Collectors.joining(SEP)) + trailingSep + this.appendFragment();
}

This comment has been minimized.

Copy link
@jroper

jroper Sep 18, 2017

Member

If this implementation is robust (I haven't looked at it carefully enough to know), then we should extract it out somewhere and use it to solve #2337.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

Thanks for the review. I'll follow up with another commit shortly.

@gmethvin There are several reasons I chose to port Python's impl. instead of using the Java Class Library.

  1. The directory separator depends on your platform. If run on Windows you would get a \\ separator. I don't believe there's any way to override this, so it would need to be replaced after.
  2. java.nio.file.Path.relativize expects path's to be directories. This isn't a big deal, but it would require the same implementation I make now where I have to strip file's from both paths and re-add them later if applicable.
  3. java.nio.file.Path.relativize cannot support .. or .. The path would need to be canonicalized. This could be done with java.io.File.getCanonicalFile or java.io.File.getCanonicalPath, but then you have to somehow handle checked exceptions for IOException.

So while it's possible to use the JCL at the end of the day (with some massaging) I just feel that it's not the appropriate thing to do given that the options available are meant to be used with the filesystem and not URL's. However, if you or the rest of the team feel that you would prefer that I can update the PR accordingly.

Move relative & canonical impl to Paths util type. Rename relative ->…
… relativeTo. Full names for dir constants. Dropped first params list from scala Call relative method.
/*
* Copyright (C) 2009-2017 Lightbend Inc. <https://www.lightbend.com>
*/
package play.libs;

This comment has been minimized.

Copy link
@gmethvin

gmethvin Sep 21, 2017

Member

Let's put this in play.core.utils, since this is a utility mainly for internal use.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

@gmethvin I updated it to the play.core.j namespace. Is that OK?

@marcospereira What documentation page and section would recommend updating? The main reference to Call is in the Routing pages (i.e. https://www.playframework.com/documentation/2.6.x/JavaRouting#Reverse-routing). There's also some refs in the JavaScript routing page (https://www.playframework.com/documentation/2.6.x/ScalaJavascriptRouting#router-resource). Although this feature isn't just for Js routing, maybe it could be mentioned there. WDYT?

@cchantep

This comment has been minimized.

Focused unit tests would be nice

This comment has been minimized.

Copy link
Contributor Author

replied Sep 22, 2017

I have test coverage on the calls into Call. See CallRelativeTest.

This comment has been minimized.

Copy link
Member

replied Sep 22, 2017

As soon as Paths is separate, it worth separate tests.

This comment has been minimized.

Copy link
Contributor Author

replied Sep 22, 2017

Alright I'll re-factor the tests to fit the new structure.

@gmethvin
Copy link
Member

left a comment

@seglo play.core.j is a weird namespace to use, since that's typically used for Java adapters to the scala API. This looks like more of a generic utility to me.

/**
* Utilities to work with URL paths.
*/
public class Paths {

This comment has been minimized.

Copy link
@gmethvin

gmethvin Sep 22, 2017

Member

Maybe add a private default constructor here since this isn't meant to be instantiated.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

@gmethvin Should I just create the play.core package under the java source tree?

@gmethvin

This comment has been minimized.

Copy link
Member

commented Sep 22, 2017

@seglo Yes, you can do that, or if you prefer you can rewrite your code in scala (I don't have a strong opinion either way).

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2017

@gmethvin OK. I should have just written it in Scala to begin with.. I had tunnel vision because Call was defined in the java source tree. Will ponder over the weekend..

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

I kept it as Java because I didn't have a compelling reason to rewrite it. I refactored the test coverage as suggested and made other misc. updates.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2017

LMK where you think the most appropriate place to document this feature would be and I'll add a commit.

The main reference to Call is in the Routing pages (i.e. https://www.playframework.com/documentation/2.6.x/JavaRouting#Reverse-routing). There's also some refs in the JavaScript routing page (https://www.playframework.com/documentation/2.6.x/ScalaJavascriptRouting#router-resource). Although this feature isn't just for Js routing, maybe it could be mentioned there. WDYT?

@gmethvin

This comment has been minimized.

Copy link
Member

commented Oct 1, 2017

@seglo I think the reverse routing section is the proper place to document it. Perhaps it would be good to add an example with a template showing both absolute and relative variants.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2017

I've added documentation to the Java and Scala guide's Routing sections. This was the last outstanding commit I had.

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

@gmethvin I fixed some of the errors from the documentation test build, but I'm not really sure what's going on the remaining errors. Could you assist?

@gmethvin

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

@seglo There were a couple problems. First is that the main template doesn't exist (maybe you forgot to commit it?). The directories also didn't match the package names, and the way twirl templates work it will automatically infer the package name from the directory.

See gmethvin@24b6713

gmethvin and others added some commits Oct 9, 2017

@seglo

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

Thanks a ton @gmethvin . I was getting mixed up between the doc package namespaces and the code sample namespaces. I doublechecked the rendered output and everything looks good. Your commit fixed the failing documentation tests.

I think we're ready for a final review. Thanks for your patience.

@gmethvin
Copy link
Member

left a comment

lgtm

@gmethvin gmethvin merged commit e74fe3d into playframework:master Oct 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

gmethvin added a commit that referenced this pull request Oct 22, 2017

Call relative and canonical path support (#7839)
* Call relative and canonical path support

* Move relative & canonical impl to Paths util type. Rename relative -> relativeTo. Full names for dir constants. Dropped first params list from scala Call relative method.

* Move Paths to play.core.j namespace

* Moved to play.core package

* Re-factor tests

* Relative routes documentation

* Docs tests fixes

* Fix package structure

* Fix refs to code samples

@marcospereira marcospereira added this to the Play 2.6.7 milestone Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.