Fixed several occurrences of inconsistent code. #7370

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
7 participants
@martinschaef
Contributor

martinschaef commented Apr 20, 2015

Fixed inconsistent code detected by our static analysis tool http://www.csl.sri.com/bixie-ws/

martinschaef added some commits Apr 20, 2015

Update HashableMarshalledValue.java
Removed wrong negation.
(testing our static analysis tool)
Update AbstractDataSourceService.java
Avoid possible NullPointerException
Update BindingType.java
fixed possible NullPointerException in line 56 for the case where localName is null.
Update InMemoryDirectoryServiceFactory.java
Fix error handling in line 97.
@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 20, 2015

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@ctomc

This comment has been minimized.

Show comment
Hide comment
@ctomc

ctomc Apr 21, 2015

Member

this is ok to test

Member

ctomc commented Apr 21, 2015

this is ok to test

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1253 is now running using a merge of 3b24116

Linux with security manager Build 1253 is now running using a merge of 3b24116

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6288 is now running using a merge of 3b24116

Linux Build 6288 is now running using a merge of 3b24116

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1395 is now running using a merge of 3b24116

Windows Build 1395 is now running using a merge of 3b24116

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1253 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:02:28

Build problems:

Process exited with code 1

Linux with security manager Build 1253 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:02:28

Build problems:

Process exited with code 1

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6288 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:02:38

Build problems:

Process exited with code 1

Linux Build 6288 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:02:38

Build problems:

Process exited with code 1

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1395 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:03:30

Build problems:

Process exited with code 1

Windows Build 1395 outcome was FAILURE using a merge of 3b24116
Summary: Tests passed: 507, ignored: 228; exit code 1 (new) Build time: 0:03:30

Build problems:

Process exited with code 1

- if (deploymentMD.getConnectionManagers() != null) {
- for (ConnectionManager cm : deploymentMD.getConnectionManagers()) {
- cm.shutdown();
+

This comment has been minimized.

@scottmarlow

scottmarlow Apr 21, 2015

Contributor

You should see the same build errors locally if you do a "./build.sh clean install" from the root WildFly folder. Seems to be a check-style complaint about line 195:
AbstractDataSourceService.java:195: Line has trailing spaces.

@scottmarlow

scottmarlow Apr 21, 2015

Contributor

You should see the same build errors locally if you do a "./build.sh clean install" from the root WildFly folder. Seems to be a check-style complaint about line 195:
AbstractDataSourceService.java:195: Line has trailing spaces.

This comment has been minimized.

@martinschaef

martinschaef Apr 21, 2015

Contributor

Changed it. Sorry, generated the patch from within github and didn't run checkstyle.

@martinschaef

martinschaef Apr 21, 2015

Contributor

Changed it. Sorry, generated the patch from within github and didn't run checkstyle.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1260 is now running using a merge of ec6a6dc

Linux with security manager Build 1260 is now running using a merge of ec6a6dc

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1402 is now running using a merge of ec6a6dc

Windows Build 1402 is now running using a merge of ec6a6dc

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6295 is now running using a merge of ec6a6dc

Linux Build 6295 is now running using a merge of ec6a6dc

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1260 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:02:32

Build problems:

Process exited with code 1

Linux with security manager Build 1260 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:02:32

Build problems:

Process exited with code 1

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6295 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:03:10

Build problems:

Process exited with code 1

Linux Build 6295 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:03:10

Build problems:

Process exited with code 1

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1402 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:04:18

Build problems:

Process exited with code 1

Windows Build 1402 outcome was FAILURE using a merge of ec6a6dc
Summary: Tests passed: 596, ignored: 279; exit code 1 Build time: 0:04:18

Build problems:

Process exited with code 1

// handle persistence.xml definition in the root of the war
if (deploymentRoot != null &&
(holder = deploymentRoot.getAttachment(PersistenceUnitMetadataHolder.PERSISTENCE_UNITS)) != null &&
holder.getPersistenceUnits().size() > 0) {
// assemble and install the PU service
puList.add(holder);
+ deploymentRootName = deploymentRoot.getRootName()

This comment has been minimized.

@scottmarlow

scottmarlow Apr 21, 2015

Contributor

looks like a missing semicolon here is causing the next build error. I didn't try to build your change either.

@scottmarlow

scottmarlow Apr 21, 2015

Contributor

looks like a missing semicolon here is causing the next build error. I didn't try to build your change either.

This comment has been minimized.

@martinschaef

martinschaef Apr 21, 2015

Contributor

Uhhh ... Mondays are always hard. Sorry about that.

@martinschaef

martinschaef Apr 21, 2015

Contributor

Uhhh ... Mondays are always hard. Sorry about that.

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1403 is now running using a merge of 32de9af

Windows Build 1403 is now running using a merge of 32de9af

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1261 is now running using a merge of 32de9af

Linux with security manager Build 1261 is now running using a merge of 32de9af

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6296 is now running using a merge of 32de9af

Linux Build 6296 is now running using a merge of 32de9af

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux with security manager Build 1261 outcome was SUCCESS using a merge of 32de9af
Summary: Tests passed: 823, ignored: 356 Build time: 0:04:55

Linux with security manager Build 1261 outcome was SUCCESS using a merge of 32de9af
Summary: Tests passed: 823, ignored: 356 Build time: 0:04:55

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Linux Build 6296 outcome was FAILURE using a merge of 32de9af
Summary: Tests failed: 1 (1 new), passed: 2852, ignored: 412, muted: 2 Build time: 0:47:04

Build problems:

Failed tests detected

Failed tests

org.jboss.as.test.integration.ejb.client.descriptor.EJBClientDescriptorTestCase.testEJBClientContextWithNoReceiversConfiguration: <no details avaliable>

Linux Build 6296 outcome was FAILURE using a merge of 32de9af
Summary: Tests failed: 1 (1 new), passed: 2852, ignored: 412, muted: 2 Build time: 0:47:04

Build problems:

Failed tests detected

Failed tests

org.jboss.as.test.integration.ejb.client.descriptor.EJBClientDescriptorTestCase.testEJBClientContextWithNoReceiversConfiguration: <no details avaliable>

@wildfly-ci

This comment has been minimized.

Show comment
Hide comment
@wildfly-ci

wildfly-ci Apr 21, 2015

Windows Build 1403 outcome was SUCCESS using a merge of 32de9af
Summary: Tests passed: 2855, ignored: 412 Build time: 0:47:51

Windows Build 1403 outcome was SUCCESS using a merge of 32de9af
Summary: Tests passed: 2855, ignored: 412 Build time: 0:47:51

@bstansberry bstansberry added the 10.x label May 4, 2015

@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry May 4, 2015

Contributor

Thanks for this. It might take a while to get in as the PR touches on a bunch of different bits of code.

Contributor

bstansberry commented May 4, 2015

Thanks for this. It might take a while to get in as the PR touches on a bunch of different bits of code.

@martinschaef

This comment has been minimized.

Show comment
Hide comment
@martinschaef

martinschaef May 4, 2015

Contributor

No problem. For me, it is more about checking if our static analysis makes sense. Btw., I'm not entirely sure if the patch for InMemoryDirectoryServiceFactory.java makes sense, because the procedure declares that it may throw something of type Exception, which would also cover RuntimeException. So maybe, this part of the patch is bogus.

Contributor

martinschaef commented May 4, 2015

No problem. For me, it is more about checking if our static analysis makes sense. Btw., I'm not entirely sure if the patch for InMemoryDirectoryServiceFactory.java makes sense, because the procedure declares that it may throw something of type Exception, which would also cover RuntimeException. So maybe, this part of the patch is bogus.

@scottmarlow

This comment has been minimized.

Show comment
Hide comment
@scottmarlow

scottmarlow Jul 2, 2015

Contributor

+1 for the JPA part of this change.

Contributor

scottmarlow commented Jul 2, 2015

+1 for the JPA part of this change.

@kwart

This comment has been minimized.

Show comment
Hide comment
@kwart

kwart Jul 2, 2015

Contributor

+1 for the InMemoryDirectoryServiceFactory.java

Contributor

kwart commented Jul 2, 2015

+1 for the InMemoryDirectoryServiceFactory.java

@pferraro

This comment has been minimized.

Show comment
Hide comment
@pferraro

pferraro Jul 13, 2015

Damn. How sloppy of me. It's worth noting that this class isn't actually used anywhere yet, but this certainly worth fixing.
Additionally, the null check isn't necessary either, since instanceof already returns false if the object is null.

Damn. How sloppy of me. It's worth noting that this class isn't actually used anywhere yet, but this certainly worth fixing.
Additionally, the null check isn't necessary either, since instanceof already returns false if the object is null.

This comment has been minimized.

Show comment
Hide comment
@pferraro

pferraro Jul 13, 2015

Otherwise, +1.

Otherwise, +1.

@bstansberry bstansberry referenced this pull request Jul 17, 2015

Merged

Static analysis fixes #7778

@bstansberry

This comment has been minimized.

Show comment
Hide comment
@bstansberry

bstansberry Jul 17, 2015

Contributor

I've opened #7778 which is a rebased version of this. I also opened wildfly/wildfly-core#894 which is for the one file here that has been moved to WildFly Core.

Thanks for this! All the issues your tool identified were valid.

Contributor

bstansberry commented Jul 17, 2015

I've opened #7778 which is a rebased version of this. I also opened wildfly/wildfly-core#894 which is for the one file here that has been moved to WildFly Core.

Thanks for this! All the issues your tool identified were valid.

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