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.

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

ColumnReader NullPointerException when finding a setSomething() method that is not a property mutator #917

Merged
merged 16 commits into from
Dec 14, 2018

Conversation

serhii-lekariev
Copy link
Contributor

@serhii-lekariev serhii-lekariev commented Dec 14, 2018

This PR addresses #912.

Before, when ColumnReader was scanning the class in an attempt to parse Columns, it has tried to get all property descriptors of that class, and once obtained, call a getReadMethod() on every descriptor, to check whether it's annotated with @Column.

If a class declares a public setSomething(Something something) method, something becomes a property, and the declared method becomes a write method for this property.

A read method, however, remains undeclared, and getReadMethod() returns a null, hence the NPE.

Now, ColumnReader checks whether a property has an accessor, before attempting to get that accessors annotations and check whether it's actually a @Column.

@serhii-lekariev
Copy link
Contributor Author

@dmdashenkov, PTAL.

@codecov
Copy link

codecov bot commented Dec 14, 2018

Codecov Report

Merging #917 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #917      +/-   ##
============================================
+ Coverage     93.36%   93.41%   +0.05%     
- Complexity     3869     3870       +1     
============================================
  Files           529      529              
  Lines         12637    12639       +2     
  Branches        703      701       -2     
============================================
+ Hits          11798    11807       +9     
+ Misses          609      605       -4     
+ Partials        230      227       -3

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev, please see my comments.

.filter(ColumnReader::hasAccessor)
.filter(ColumnReader::accessorIsAnnotated)
.map(PropertyDescriptor::getReadMethod)
.forEach(readMethod -> entityColumns.add(EntityColumn.from(readMethod)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a combination of map and collect instead. forEach must be stateless. In practice, in order to be honest with the functional style coding, forEach is almost never applicable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, you may extract the method from the descriptor as the first step, filter out nulls and not annotated methods and collect them at once. This is also a performance improvement, since PropertyDescriptor.getReadMethod() looks quite complex and not necessarily fast.

@@ -114,6 +115,14 @@ void fromImplementedInterface() {
}
}

@Test
@DisplayName("not confuse a `setSomething()` method with a property mutator")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("not confuse a `setSomething()` method with a property mutator")
@DisplayName("not confuse a setter method with a property mutator")

}

@Column
public Integer getFortyThree(){
Copy link
Contributor

Choose a reason for hiding this comment

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

The presence of this @Column makes the test a bit confusing. Can we just drop it and assert that there are no columns?

@serhii-lekariev
Copy link
Contributor Author

@dmdashenkov, PTAL again.

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev, please see my new comments.

@serhii-lekariev
Copy link
Contributor Author

@dmdashenkov, PTAL again.

Copy link
Contributor

@dmdashenkov dmdashenkov left a comment

Choose a reason for hiding this comment

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

@serhii-lekariev, please address my final comment. LGTM then.

checkRepeatedColumnNames(entityColumns);
return entityColumns;
PropertyDescriptor[] propertyDescriptors = entityDescriptor.getPropertyDescriptors();
ImmutableSet<EntityColumn> annotatedColumns = Arrays
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ImmutableSet<EntityColumn> annotatedColumns = Arrays
ImmutableSet<EntityColumn> columns = Arrays

@serhii-lekariev serhii-lekariev merged commit 479fd46 into master Dec 14, 2018
@serhii-lekariev serhii-lekariev deleted the column-reader-npe-fix branch December 14, 2018 16:08
@armiol armiol mentioned this pull request Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants