Skip to content

Add test for field ordering#1042

Merged
mkurz merged 1 commit intoplayframework:mainfrom
levinson:field-ordering-scala3-bug
Jun 30, 2025
Merged

Add test for field ordering#1042
mkurz merged 1 commit intoplayframework:mainfrom
levinson:field-ordering-scala3-bug

Conversation

@levinson
Copy link

@levinson levinson commented May 11, 2024

Pull Request Checklist

  • Have you read through the contributor guidelines?
  • Have you squashed your commits?
  • Have you added copyright headers to new files? N/A
  • Have you updated the documentation? N/A
  • Have you added tests for any changed functionality? N/A

Purpose

This PR demonstrates #1038. The new test fails due to serialized field order when compiled with Scala 3:

[info] - should preserve field ordering *** FAILED *** (33 milliseconds)
[info]   "{"[a":42,"m1":"foo","b":"baz","c":"bar","z":1.2]}" did not equal "{"[m1":"foo","c":"bar","z":1.2,"a":42,"b":"baz"]}" (JsonSharedSpec.scala:454)

@levinson levinson force-pushed the field-ordering-scala3-bug branch from cff4e97 to 588157c Compare May 11, 2024 20:38
cchantep
cchantep reviewed May 20, 2024
View reviewed changes
play-json/shared/src/test/scala/play/api/libs/json/JsonSharedSpec.scala
"preserve field ordering" in {
val model = Model(Some("foo"), "bar", Some(1.2), 42, Some("baz"))
val serialized = Json.stringify(Json.toJson(model))
serialized.mustEqual("""{"m1":"foo","c":"bar","z":1.2,"a":42,"b":"baz"}""")
Copy link
Member

@cchantep cchantep May 20, 2024

Choose a reason for hiding this comment

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

Suggested change
serialized.mustEqual("""{"m1":"foo","c":"bar","z":1.2,"a":42,"b":"baz"}""")
serialized.mustEqual("""{"m1":"foo","c":"bar","z":1.2,"a":42,"b":"baz"}""")

@jonaskoelker-jypo
Copy link

jonaskoelker-jypo commented Aug 1, 2024

Would it make sense to test the field ordering at the JsObject level in addition to the String level?

I note the comment on the fields method on JsObject: it gives the fields in order. Based on this, I would—naively?—expect the Seq returned by fields to be in declaration order when using the Json.writes macro and friends. It would be a real nicety to have a test for that as well.

play-json/play-json/shared/src/main/scala/play/api/libs/json/JsValue.scala

Lines 116 to 123 in 85a8aa0

case class JsObject(
private[json] val underlying: Map[String, JsValue]
) extends JsValue {
/**
* The fields of this JsObject in the order passed to the constructor
*/
def fields: collection.Seq[(String, JsValue)] = underlying.toSeq

[On the other hand: I can argue that a JsObject doesn't contain any information about the field declaration order except the ordering of the ImmutableLinkedHashMap in underlying. Ergo: if it's wrong at the JsObject level there's no way Json.stringify can correct it, and thus this test case implicitly also verifies that underlying is ordered correctly. That feels a bit indirect for my tastes—I lean towards an independent test of that—but I figured it deserved to be mentioned.]

@mkurz
Copy link
Member

mkurz commented Jun 30, 2025

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jun 30, 2025

rebase

✅ Branch has been successfully rebased

@mkurz mkurz force-pushed the field-ordering-scala3-bug branch from 588157c to c8946b3 Compare June 30, 2025 14:31
@mkurz
Copy link
Member

mkurz commented Jun 30, 2025

The test now passes since #1038 got merged. There is a test in that PR as well but I will merge this one too.

@mkurz mkurz merged commit 7fd06dd into playframework:main Jun 30, 2025
14 checks passed
@mkurz
Copy link
Member

mkurz commented Jun 30, 2025

https://github.com/Mergifyio backport 3.0.x 2.10.x

@mergify
Copy link
Contributor

mergify bot commented Jun 30, 2025 •
edited
Loading

backport 3.0.x 2.10.x

✅ Backports have been created

Details

This was referenced Jun 30, 2025
Merged
Merged
@pjfanning pjfanning mentioned this pull request Jul 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cchantep cchantep cchantep left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@levinson @jonaskoelker-jypo @mkurz @cchantep