Binders for OptionalInt, OptionalLong, OptionalDouble

[PromanSEW]

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 referenced any issues you're fixing using commit message keywords?
• 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?
• Have you added tests for any changed functionality?

Helpful things

Fixes

Fixes #10816

Purpose

Add binders for Java OptionalInt, OptionalLong, OptionalDouble

References

#10816

[mkurz]

Thanks for the pull request.
Please see my inside the code diff: You need to add the javascriptUnbind methods for each binder.

Plus two more things 👇

First you also need to add JavascriptLiterals for each OptionalXXX type. Like we have for Optional here:

playframework/core/play/src/main/scala/play/api/mvc/Binders.scala

Lines 258 to 262 in 9852822

	/**
	* Convert a Java Optional to Javascript literal (use "null" for an empty Optional)
	*/
	implicit def literalJavaOption[T](implicit jsl: JavascriptLiteral[T]): JavascriptLiteral[Optional[T]] =
	(value: Optional[T]) => value.asScala.map(jsl.to(_)).getOrElse("null")

E.g. for OptionalInt you have to add:

  /**
   * Convert a Java OptionalInt to Javascript literal (use "null" for an empty OptionalInt)
   */
  implicit def literalJavaOptionalInt: JavascriptLiteral[OptionalInt] =
    (value: OptionalInt) => value.asScala.map(_.toString).getOrElse("null")

Same for OptionalLong and OptionalDouble please.

Second, please add tests, I think that should be here: https://github.com/playframework/playframework/tree/master/dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-routes-compilation
E.g. look in the conf/routes file, there is a route for an Optional param, which points to this action method.
Finally a test is defined here.
Please do similar tests for OptionalInt, OptionalDouble and OptionalLong.
Also it would be good to add a test with default values, see here.

Bonus: You could also add tests for the Javascript reverse routes (to test the literals you add, etc.) here: https://github.com/playframework/playframework/blob/28f19894deae8712682352acf53d99eb4a24da46/dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-routes-compilation/tests/assets/JavaScriptRouterSpec.js Just re-use the routes you will add for the tests above.
(Also, for this to work I think you need to the reverse routes here:

playframework/dev-mode/sbt-plugin/src/sbt-test/play-sbt-plugin/routes-compiler-routes-compilation/app/utils/JavaScriptRouterGenerator.scala

Lines 15 to 24 in 9852822

	.JavaScriptReverseRouter(
	"jsRoutes",
	None,
	"localhost",
	Assets.versioned,
	Application.index,
	Application.post,
	Application.withParam,
	Application.takeBool
	)

)

Thanks!

[PromanSEW]

Unfortunately, I could not run tests locally because of error:

D:\Documents\GitHub\playframework\dev-mode\routes-compiler\src\main\scala\play\routes\compiler\RoutesGenerator.scala:172:5
not found: value static
    static.twirl

D:\Documents\GitHub\playframework\dev-mode\routes-compiler\src\main\scala\play\routes\compiler\RoutesGenerator.scala:159:5
not found: value inject
    inject.twirl

D:\Documents\GitHub\playframework\dev-mode\routes-compiler\src\main\scala\play\routes\compiler\RoutesGenerator.scala:194:11
not found: value static
          static.twirl

D:\Documents\GitHub\playframework\dev-mode\routes-compiler\src\main\scala\play\routes\compiler\RoutesGenerator.scala:222:11
not found: value static
          static.twirl

D:\Documents\GitHub\playframework\dev-mode\routes-compiler\src\main\scala\play\routes\compiler\RoutesGenerator.scala:251:11
not found: value static
          static.twirl.javaWrappers(sourceInfo, namespace, packageName, controllers).body

[PromanSEW]

I'm not sure about this test, is it right?

[PromanSEW]

I think it should be like:

    it("should generate a url for default parameters", function() {
        var data = jsRoutes.controllers.Application.takeOptionalIntWithDefault();
        assert.equal("/take-joptint-d", data.url);
    });

Or:

    it("should add non-native JavaScript parameters to the query string", function() {
        var data = jsRoutes.controllers.Application.takeOptionalInt(123);
        assert.equal("/take-joptint?x=123", data.url);
    });

[mkurz]

I think it should be like:

Yeah, I think your comment is right. However, what do you think of changing the description of the tests a bit and add three more, like:

    it("should generate a url for an OptionalInt parameter (which has a default)", function() {
        var data = jsRoutes.controllers.Application.takeOptionalIntWithDefault();
        assert.equal("/take-joptint-d", data.url);
    });

    it("should generate a url for an OptionalInt parameter (which has a default) when default param value is passed", function() {
        var data = jsRoutes.controllers.Application.takeOptionalIntWithDefault(123);
        assert.equal("/take-joptint-d", data.url);
    });

    it("should add an OptionalInt parameter (which has a default) to the query string", function() {
        var data = jsRoutes.controllers.Application.takeOptionalIntWithDefault(987);
        assert.equal("/take-joptint-d?x=987", data.url);
    });

    it("should generate a url for an OptionalInt parameter", function() {
        var data = jsRoutes.controllers.Application.takeOptionalInt();
        assert.equal("/take-joptint", data.url);
    });

    it("should add an OptionalInt parameter to the query string", function() {
        var data = jsRoutes.controllers.Application.takeOptionalInt(123);
        assert.equal("/take-joptint?x=123", data.url);
    });

[PromanSEW]

I don't know what test is failing. Travis does not write that

[PromanSEW]

What's wrong now? -_-

[info] [info] bind java.util.OptionalInt parameter with default value from the query string
[info] [info]   + successfully
[info] [error]   x when there is a parameter but without value (=empty string)
[info] [error]    'emptyOptionalInt' != '123' (RouterSpec.scala:565)
[info] [error]   x when there is a parameter but without value (=empty string) and without equals sign
[info] [error]    'emptyOptionalInt' != '123' (RouterSpec.scala:565)
[info] [error]   x when there is no parameter at all
[info] [error]    'emptyOptionalInt' != '123' (RouterSpec.scala:569)
[info] [info] bind java.util.OptionalLong parameter with default value from the query string
[info] [info]   + successfully
[info] [error]   x when there is a parameter but without value (=empty string)
[info] [error]    'emptyOptionalLong' != '123' (RouterSpec.scala:579)
[info] [error]   x when there is a parameter but without value (=empty string) and without equals sign
[info] [error]    'emptyOptionalLong' != '123' (RouterSpec.scala:579)
[info] [error]   x when there is no parameter at all
[info] [error]    'emptyOptionalLong' != '123' (RouterSpec.scala:583)
[info] [info] bind java.util.OptionalDouble parameter with default value from the query string
[info] [info]   + successfully
[info] [error]   x when there is a parameter but without value (=empty string)
[info] [error]    'emptyOptionalDouble' != '1.23' (RouterSpec.scala:593)
[info] [error]   x when there is a parameter but without value (=empty string) and without equals sign
[info] [error]    'emptyOptionalDouble' != '1.23' (RouterSpec.scala:593)
[info] [error]   x when there is no parameter at all
[info] [error]    'emptyOptionalDouble' != '1.23' (RouterSpec.scala:597)

[PromanSEW]

@mkurz any updates?

[mkurz]

@Mergifyio backport 2.8.x

[mergify]

Command backport 2.8.x: pending

Waiting for the pull request to get merged

[mergify]

Command backport 2.8.x: success

Backports have been created

• #10830 Binders for OptionalInt, OptionalLong, OptionalDouble (backport #10825) has been created for branch 2.8.x