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.

Sign up for GitHub

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

Remove unnecessary error generation when JsUndefined.asOpt is used #1112

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove unnecessary error generation when JsUndefined.asOpt is used #1112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9165181
Removed unnecessary error generation in the JsUndefined class when th…
Dubinka94 Dec 17, 2024
ec302fd
Added test for JsUndefined.asOpt function
Dubinka94 Jan 16, 2025
5e7eec6
Formatting
Dubinka94 Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions play-json/shared/src/main/scala/play/api/libs/json/JsLookup.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,10 @@ case class JsDefined(value: JsValue) extends AnyVal with JsLookupResult
* Represent a missing Json value.
*/
final class JsUndefined(err: => String) extends JsLookupResult {
def error = err
def validationError = JsonValidationError(error)
override def toString = s"JsUndefined($err)"
def error = err
def validationError = JsonValidationError(error)
override def toString = s"JsUndefined($err)"
override def asOpt[T](implicit fjs: Reads[T]): Option[T] = None
Copy link
Member

Choose a reason for hiding this comment

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

👍 Make sure it's test covered

Copy link
Author

Choose a reason for hiding this comment

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

What kind of test would you like me to add?
Testing that asOpt works as expected for both valid and invalid keys is already covered by other tests. Benchmarking to ensure performance stays within a certain threshold might be unreliable due to different test environments.
And I don’t have access to the intermediate object created inside asOpt to verify whether it was created or not in case of the older implementation.

Copy link
Member

Choose a reason for hiding this comment

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

There is no direct test of JsUndefined whereas it can be easily done.

}

object JsUndefined {
Expand Down
27 changes: 27 additions & 0 deletions play-json/shared/src/test/scala/play/api/libs/json/JsLookupSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (C) from 2022 The Play Framework Contributors <https://github.com/playframework>, 2011-2021 Lightbend Inc. <https://www.lightbend.com>
*/

package play.api.libs.json

import play.api.libs.json.Json._

import org.scalatest.matchers.must.Matchers
import org.scalatest.wordspec.AnyWordSpec

class JsLookupSpec extends AnyWordSpec with Matchers {
"JsLookupResult" should {
val obj = Json.obj(
"field" -> 123
)
val result = obj \ "missingField"

"return JsUndefined when a key is missing" in {
result mustBe a[JsUndefined]
}

"return None when calling asOpt on JsUndefined" in {
result.asOpt[Int].mustEqual(None)
}
}
}
Loading