Issue #3187: aligned setters with String collection fields and broke old compatibility #3189

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@rnveach
Member

rnveach commented May 17, 2016

Issue #3187

basically changed only:

void set(String s) {
    field = s.split(",");
}

into:

void set(String... s) {
    field = s;
}

Braking Compatibility:
As stated in #3189 (comment), old code didn't properly split comma delimited inputs from the configuration. Spaces were left in and these caused properties like 1, 2 to be seen as 1 and <space>2. This caused checks like IllegalInstantiation to not identify classes properly and hid what should have been violations.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 17, 2016

Current coverage is 100%

Merging #3189 into master will not change coverage

@@             master      #3189   diff @@
==========================================
  Files           275        275          
  Lines         13920      13910    -10   
  Methods           0          0          
  Messages          0          0          
  Branches       3193       3191     -2   
==========================================
- Hits          13920      13910    -10   
  Misses            0          0          
  Partials          0          0          

Powered by Codecov. Last updated by ff896f7...a52ad2e

codecov-io commented May 17, 2016

Current coverage is 100%

Merging #3189 into master will not change coverage

@@             master      #3189   diff @@
==========================================
  Files           275        275          
  Lines         13920      13910    -10   
  Methods           0          0          
  Messages          0          0          
  Branches       3193       3191     -2   
==========================================
- Hits          13920      13910    -10   
  Misses            0          0          
  Partials          0          0          

Powered by Codecov. Last updated by ff896f7...a52ad2e

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 18, 2016

Member

@romani
It looks like travis failure with XWiki is showing an issue with how IllegalInstantiation's setter isn't working as users thought.

Travis Error:
https://travis-ci.org/checkstyle/checkstyle/jobs/130923497#L7360

[ERROR] /home/travis/build/checkstyle/checkstyle/xwiki-commons/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/Sax2Dom.java:152:33: Instantiation of java.lang.String should be avoided. [IllegalInstantiation]

Configuration:
https://github.com/xwiki/xwiki-commons/blob/02f7260a982f8e19fcccaafa3fd4261220ffe079/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml#L165-L167

<module name="IllegalInstantiation">
  <property name="classes" value="java.lang.Boolean, java.lang.String"/>

Code:
https://github.com/xwiki/xwiki-commons/blob/c6ec6b4559977667e020090313580dd037f654fa/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/Sax2Dom.java#L152

final String text = new String(ch, start, length);

Issue is the space after the comma in the old code. We aren't taking it out so instead of looking for java.lang.String we were incorrectly looking for <space>java.lang.String.
Our unit tests don't have this scenario, hence why it went unnoticed.

Member

rnveach commented May 18, 2016

@romani
It looks like travis failure with XWiki is showing an issue with how IllegalInstantiation's setter isn't working as users thought.

Travis Error:
https://travis-ci.org/checkstyle/checkstyle/jobs/130923497#L7360

[ERROR] /home/travis/build/checkstyle/checkstyle/xwiki-commons/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/Sax2Dom.java:152:33: Instantiation of java.lang.String should be avoided. [IllegalInstantiation]

Configuration:
https://github.com/xwiki/xwiki-commons/blob/02f7260a982f8e19fcccaafa3fd4261220ffe079/xwiki-commons-tools/xwiki-commons-tool-verification-resources/src/main/resources/checkstyle.xml#L165-L167

<module name="IllegalInstantiation">
  <property name="classes" value="java.lang.Boolean, java.lang.String"/>

Code:
https://github.com/xwiki/xwiki-commons/blob/c6ec6b4559977667e020090313580dd037f654fa/xwiki-commons-core/xwiki-commons-xml/src/main/java/org/xwiki/xml/Sax2Dom.java#L152

final String text = new String(ch, start, length);

Issue is the space after the comma in the old code. We aren't taking it out so instead of looking for java.lang.String we were incorrectly looking for <space>java.lang.String.
Our unit tests don't have this scenario, hence why it went unnoticed.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 21, 2016

Member

good catch.

but right now we use xwiki code directly from their github.
Two options to resolve build failure:

  1. contact xwiki and report problems to them to let them fix them now instead of during future upgrade to new version
  2. make a fork of their project and switch our Travis to our fork, our fork need to be updated to avoid violations.
Member

romani commented May 21, 2016

good catch.

but right now we use xwiki code directly from their github.
Two options to resolve build failure:

  1. contact xwiki and report problems to them to let them fix them now instead of during future upgrade to new version
  2. make a fork of their project and switch our Travis to our fork, our fork need to be updated to avoid violations.

@rnveach rnveach referenced this pull request in xwiki/xwiki-commons May 22, 2016

Merged

[Misc] Fix violations for bug in 'IllegalInstantiationCheck' #15

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 22, 2016

Member

PR in XWiki created, waiting for their response.

Member

rnveach commented May 22, 2016

PR in XWiki created, waiting for their response.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 23, 2016

Member

PR was merged, rebased to see if travis would pass.

@romani I believe XWiki will still fail.
Change was introduced into XWiki's master, but we are validating against release xwiki-commons-8.0-milestone-1.

Member

rnveach commented May 23, 2016

PR was merged, rebased to see if travis would pass.

@romani I believe XWiki will still fail.
Change was introduced into XWiki's master, but we are validating against release xwiki-commons-8.0-milestone-1.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 26, 2016

Member

@rnveach , lets update our contibutions project to reference to commit hash where such fix is merged instead of "xwiki-commons-8.0-milestone-1" - it will let CI pass.

Member

romani commented May 26, 2016

@rnveach , lets update our contibutions project to reference to commit hash where such fix is merged instead of "xwiki-commons-8.0-milestone-1" - it will let CI pass.

@rnveach

This comment has been minimized.

Show comment
Hide comment
Member

rnveach commented May 27, 2016

@romani Done.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 27, 2016

Member

@rnveach , I am ready to merge.

please change commits prefix from Issue #3187: to Pull #3189 as current fixes are not about UT fixes - they are braking compatibility (as all plugins might need to update their metadata). We need to update Pull topic to be descriptive about a change, Pull description need to state braking changes.

Member

romani commented May 27, 2016

@rnveach , I am ready to merge.

please change commits prefix from Issue #3187: to Pull #3189 as current fixes are not about UT fixes - they are braking compatibility (as all plugins might need to update their metadata). We need to update Pull topic to be descriptive about a change, Pull description need to state braking changes.

@romani romani added this to the 6.19 milestone May 27, 2016

romani added a commit that referenced this pull request May 27, 2016

@rnveach rnveach changed the title from Issue #3187: aligned setters with String collection fields to Issue #3187: aligned setters with String collection fields and broke old compatibility May 27, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani May 27, 2016

Member

merged as FF

Member

romani commented May 27, 2016

merged as FF

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 27, 2016

Member

Done.

Member

rnveach commented May 27, 2016

Done.

@rnveach rnveach deleted the rnveach:validate_2 branch May 27, 2016

@tsjensen

This comment has been minimized.

Show comment
Hide comment
@tsjensen

tsjensen Jun 3, 2016

Contributor

Can you please explain how this PR breaks backward compatibility?
From what I can see, the change only fixes the space-around-separator problem, so any previously valid configuration files should continue to work, shouldn't they?

Contributor

tsjensen commented Jun 3, 2016

Can you please explain how this PR breaks backward compatibility?
From what I can see, the change only fixes the space-around-separator problem, so any previously valid configuration files should continue to work, shouldn't they?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 3, 2016

Member

yes, xml configuration should work as it was.
Behavior is changed.
Signature of Check's public methods is changed - this kind of breaking compatibility for some tools.
So I decided to mark it like this just in case.

Member

romani commented Jun 3, 2016

yes, xml configuration should work as it was.
Behavior is changed.
Signature of Check's public methods is changed - this kind of breaking compatibility for some tools.
So I decided to mark it like this just in case.

romani added a commit that referenced this pull request Jun 7, 2016

romani added a commit that referenced this pull request Jun 24, 2016

romani added a commit that referenced this pull request Jun 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment