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

Jump to bottom

scala.xml.Utility removes attributes when sorting. #7074

Closed
scabug opened this issue Feb 5, 2013 · 14 comments
Closed

scala.xml.Utility removes attributes when sorting. #7074

scabug opened this issue Feb 5, 2013 · 14 comments
Assignees
@khernyo
Milestone
2.10.2

Comments

@scabug
Copy link

scabug commented Feb 5, 2013 •
edited by retronym
Loading

In 2.10.0 the Utility.sort(md: MetaData) function has broken. An example of when the problem occurs is below, it is for a case where alphabetically the first attribute comes after the proceeding attributes.

scala> val node:Node = <a d="1" b="2" c="3"></a>
node: scala.xml.Node = <a d="1" b="2" c="3"></a>

scala> Utility.sort(node)
res8: scala.xml.Node = <a b="2" d="1"/> //attribute c is missing

This works in 2.9.2 where the only difference in any of the sorting functions is in the MetaData sorting function. The value that is returned after sorting some attributes is:

// 2.9.2 Utility.scala
smaller.append( Null ).append(md.copy ( greater ))
// 2.10.0 Utility.scala
smaller.copy(md.copy ( greater ))

In 2.10.0 omitting smaller.append( Null ) loses attributes in some cases. I believe this change should be reverted to 2.9.2.
@scabug
Copy link
Author

scabug commented Feb 5, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7074?orig=1
Reporter: Matthew Pickering (matthedude)
Affected Versions: 2.10.0

@scabug
Copy link
Author

scabug commented Feb 13, 2013

@ViniciusMiana said:
Digging further, I believe the bug is in the copy behaviour:

val a = new UnprefixedAttribute("a","3", Null)
val b = new UnprefixedAttribute("b","3", a)
val c = new UnprefixedAttribute("c","1", Null)
val d = new UnprefixedAttribute("d","5", c)

d: scala.xml.UnprefixedAttribute = d="5" c="1"
b: scala.xml.UnprefixedAttribute = b="3" a="3"

scala> d.copy(b)
res32: scala.xml.UnprefixedAttribute = d="5" b="3" a="3"

This happens because the copy method only takes the first attribute
from d, so c or any other attributes are gone.
Is this the expected behaviour for copy?

@scabug
Copy link
Author

scabug commented Feb 13, 2013

@acruise said:
Good catch, thanks! I have very little time at the moment, could someone please review this patch? For me at least it does the right thing:

defined class UnprefixedAttribute

scala> :paste
// Entering paste mode (ctrl-D to finish)

val a = new UnprefixedAttribute("a","3", Null)
val b = new UnprefixedAttribute("b","3", a)
val c = new UnprefixedAttribute("c","1", Null)
val d = new UnprefixedAttribute("d","5", c)

// Exiting paste mode, now interpreting.

a: UnprefixedAttribute = a="3"
b: UnprefixedAttribute = b="3" a="3"
c: UnprefixedAttribute = c="1"
d: UnprefixedAttribute = d="5" c="1"

scala> d.copy(b)
res1: UnprefixedAttribute = d="5" c="1" b="3" a="3"

@scabug
Copy link
Author

scabug commented Feb 13, 2013

@acruise said:
I believe this change should be binary-compatible, so there's some chance of getting it into 2.10.x.

@scabug
Copy link
Author

scabug commented Feb 13, 2013

@ViniciusMiana said:
I can take care of that. I just want to know what copy is supposed to do.
If it is supposed to keep the other attributes (next), than what you did looks right. However if we look the scaladoc:

abstract def copy(next: MetaData): MetaData
returns a copy of this MetaData item with next field set to argument.

I am not sure what it is supposed to do...

@scabug
Copy link
Author

scabug commented Feb 14, 2013

@acruise said:
Now I'm not sure that my version of the copy method is right. Typically, for case classes, copy is used to update fields by replacing their values, not merging with previous values. I'll have another look today...

@scabug
Copy link
Author

scabug commented Feb 14, 2013

@ViniciusMiana said:
Who should we ask? I tried the scala-internals, but got no response...

@scabug
Copy link
Author

scabug commented Feb 15, 2013

@acruise said:
Szabolcs (added as watcher), can you comment on this? Thanks!

@scabug
Copy link
Author

scabug commented Feb 17, 2013

@khernyo said:
I'm not familiar with the xml implementation, but if the fix is correct, it should be applied to PrefixedAttribute.scala, too.

I just checked and the attached patch causes a stack overflow for me: https://github.com/khernyo/scala/commits/issue/7074
This is on branch 2.10.x
What did I miss?

The test also checks if the sort method retains the "" vs "" format, but I don't think it's important.

@scabug
Copy link
Author

scabug commented Feb 20, 2013

@acruise said (edited on Feb 20, 2013 12:36:51 AM UTC):
Szabolcs, I namedropped you because I thought it was your fix that changed the behaviour between 2.9 and 2.10. My patch is no good, please ignore it. :)

@scabug
Copy link
Author

scabug commented Feb 20, 2013

@khernyo said:
It's possible it was my fix that caused this bug. I will take a closer look this weekend.

@scabug
Copy link
Author

scabug commented Feb 21, 2013

@khernyo said:
Pull request: scala/scala#2149

@scabug
Copy link
Author

scabug commented Oct 10, 2013

@som-snytt said:
Should be closed, fixed in 2.10.2?

@scabug
Copy link
Author

scabug commented Oct 10, 2013

Matthew Pickering (matthedude) said:
Yes it's fixed we've moved to using 2.10.2.

@scabug scabug closed this as completed Oct 10, 2013
@scabug scabug added this to the 2.10.2 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@khernyo khernyo

Labels
None yet
Projects
None yet
Milestone
2.10.2
Development

No branches or pull requests
2 participants
@khernyo @scabug