`ColumnReader` `NullPointerException` when finding a `setSomething()` method that is not a property mutator by serhii-lekariev · Pull Request #917 · SpineEventEngine/core-jvm

serhii-lekariev · 2018-12-14T12:54:21Z

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.

# Conflicts: # testutil-client/src/main/java/io/spine/testing/client/TestActorRequestFactory.java

…Engine/core-java into column-reader-npe-fix

serhii-lekariev · 2018-12-14T13:48:16Z

@dmdashenkov, PTAL.

codecov · 2018-12-14T14:15:25Z

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

dmdashenkov

@serhii-lekariev, please see my comments.

dmdashenkov · 2018-12-14T14:24:15Z

server/src/main/java/io/spine/server/entity/storage/ColumnReader.java

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


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.

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.

dmdashenkov · 2018-12-14T14:25:29Z

server/src/test/java/io/spine/server/entity/storage/ColumnReaderTest.java

    }

+    @Test
+    @DisplayName("not confuse a `setSomething()` method with a property mutator")


Suggested change

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

@DisplayName("not confuse a setter method with a property mutator")

dmdashenkov · 2018-12-14T14:26:47Z

server/src/test/java/io/spine/server/entity/storage/given/ColumnsTestEnv.java

+        }
+
+        @Column
+        public Integer getFortyThree(){


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 · 2018-12-14T14:51:45Z

@dmdashenkov, PTAL again.

dmdashenkov

@serhii-lekariev, please see my new comments.

server/src/main/java/io/spine/server/entity/storage/ColumnReader.java

server/src/test/java/io/spine/server/entity/storage/ColumnReaderTest.java

server/src/test/java/io/spine/server/entity/storage/given/ColumnsTestEnv.java

serhii-lekariev · 2018-12-14T15:36:10Z

@dmdashenkov, PTAL again.

dmdashenkov

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

dmdashenkov · 2018-12-14T15:49:18Z

server/src/main/java/io/spine/server/entity/storage/ColumnReader.java

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


Suggested change

ImmutableSet<EntityColumn> annotatedColumns = Arrays

ImmutableSet<EntityColumn> columns = Arrays

serhii-lekariev and others added 4 commits December 14, 2018 12:38

Omit passing both zone ID and offset and calculate offset based on ID

e8b86a9

Setup environment to reproduce #912

4da7f96

Fix invalid attempts to obtain a getter method when it's not present

0e4acaf

Merge remote-tracking branch 'origin/master' into column-reader-npe-fix

095bc17

# Conflicts: # testutil-client/src/main/java/io/spine/testing/client/TestActorRequestFactory.java

serhii-lekariev self-assigned this Dec 14, 2018

serhii-lekariev requested a review from dmdashenkov December 14, 2018 12:54

serhii-lekariev and others added 3 commits December 14, 2018 15:29

Setup environment to reproduce #912

b1e383d

Fix invalid attempts to obtain a getter method when it's not present

fd5e8ba

Merge branch 'column-reader-npe-fix' of https://github.com/SpineEvent…

f4b53dd

…Engine/core-java into column-reader-npe-fix

Substitute iterator loop with a stream chain call

6830496

dmdashenkov suggested changes Dec 14, 2018

View reviewed changes

serhii-lekariev added 2 commits December 14, 2018 16:47

Change order of operations in the stream chain

10eef1b

Change documentation wording

ac21461

Kill a redundant linebreak

836c891

dmdashenkov suggested changes Dec 14, 2018

View reviewed changes

serhii-lekariev added 4 commits December 14, 2018 17:19

Fix documentation wording

5f6892d

Remove redundant utility method

b1bba8d

Remove unwanted changes

507e68e

Fix formatting

145ca40

dmdashenkov approved these changes Dec 14, 2018

View reviewed changes

annotatedColumns to columns local variable rename

bd2d068

serhii-lekariev merged commit 479fd46 into master Dec 14, 2018

serhii-lekariev deleted the column-reader-npe-fix branch December 14, 2018 16:08

serhii-lekariev mentioned this pull request Dec 17, 2018

Unexpected NPE when scanning for entity columns #912

Closed

armiol mentioned this pull request Dec 20, 2018

Publish 1.0.0-pre2. #923

Merged

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

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

Conversation

serhii-lekariev commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhii-lekariev commented Dec 14, 2018

Uh oh!

codecov bot commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

serhii-lekariev commented Dec 14, 2018

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhii-lekariev commented Dec 14, 2018

Uh oh!

dmdashenkov left a comment

Choose a reason for hiding this comment

Uh oh!

dmdashenkov Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

serhii-lekariev commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 14, 2018 •

edited

Loading