From d739242768f3e0a2dc71c004d3e7006727b91dd2 Mon Sep 17 00:00:00 2001 From: Nithin Sujir <118742+nithinsujir@users.noreply.github.com> Date: Tue, 25 Sep 2018 17:40:59 -0700 Subject: [PATCH] spanner: Options: Fix null dereference, expand test coverage (#3706) The equals code does not handle the comparison of two Options objects where only one of them has either limit, pageSize or prefetchChunks defined. The argument to Object.equals() takes for instance limit() and this tries to deref the null to convert it to a long. Add hasXXX() checks before calling Object.equals(). This patch also removes functions that will not be called since they are internal. This expands coverage of Options to 100%. --- .../com/google/cloud/spanner/Options.java | 25 +--- .../com/google/cloud/spanner/OptionsTest.java | 130 ++++++++++++++++++ 2 files changed, 136 insertions(+), 19 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java index 256968798fdd..3446b51970ac 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java @@ -110,10 +110,6 @@ static final class FlowControlOption extends InternalOption implements ReadAndQu this.prefetchChunks = prefetchChunks; } - int prefetchChunks() { - return prefetchChunks; - } - @Override void appendToOptions(Options options) { options.prefetchChunks = prefetchChunks; @@ -202,10 +198,13 @@ public boolean equals(Object o) { } Options that = (Options) o; - return (!hasLimit() && !that.hasLimit() || Objects.equals(limit(), that.limit())) + return (!hasLimit() && !that.hasLimit() + || hasLimit() && that.hasLimit() && Objects.equals(limit(), that.limit())) && (!hasPrefetchChunks() && !that.hasPrefetchChunks() - || Objects.equals(prefetchChunks(), that.prefetchChunks())) - && (!hasPageSize() && !that.hasPageSize() || Objects.equals(pageSize(), that.pageSize())) + || hasPrefetchChunks() && that.hasPrefetchChunks() + && Objects.equals(prefetchChunks(), that.prefetchChunks())) + && (!hasPageSize() && !that.hasPageSize() + || hasPageSize() && that.hasPageSize() && Objects.equals(pageSize(), that.pageSize())) && Objects.equals(pageToken(), that.pageToken()) && Objects.equals(filter(), that.filter()); } @@ -266,10 +265,6 @@ static class LimitOption extends InternalOption implements ReadOption { this.limit = limit; } - long limit() { - return limit; - } - @Override void appendToOptions(Options options) { options.limit = limit; @@ -283,10 +278,6 @@ static class PageSizeOption extends InternalOption implements ListOption { this.pageSize = pageSize; } - int pageSize() { - return pageSize; - } - @Override void appendToOptions(Options options) { options.pageSize = pageSize; @@ -300,10 +291,6 @@ static class PageTokenOption extends InternalOption implements ListOption { this.pageToken = pageToken; } - String pageToken() { - return pageToken; - } - @Override void appendToOptions(Options options) { options.pageToken = pageToken; diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java index 4b627526a693..ea88158c1e41 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java @@ -67,5 +67,135 @@ public void allOptionsAbsent() { Options options = Options.fromReadOptions(); assertThat(options.hasLimit()).isFalse(); assertThat(options.hasPrefetchChunks()).isFalse(); + assertThat(options.hasFilter()).isFalse(); + assertThat(options.hasPageToken()).isFalse(); + assertThat(options.toString()).isEqualTo(""); + assertThat(options.equals(options)).isTrue(); + assertThat(options.equals(null)).isFalse(); + assertThat(options.equals(this)).isFalse(); + + assertThat(options.hashCode()).isEqualTo(31); + } + + @Test + public void listOptTest() { + int pageSize = 3; + String pageToken = "ptok"; + String filter = "env"; + Options opts = Options + .fromListOptions( + Options.pageSize(pageSize), Options.pageToken(pageToken), Options.filter(filter)); + + assertThat(opts.toString()).isEqualTo("pageSize: " + Integer.toString(pageSize) + + " pageToken: " + pageToken + " filter: " + filter + " "); + + assertThat(opts.hasPageSize()).isTrue(); + assertThat(opts.hasPageToken()).isTrue(); + assertThat(opts.hasFilter()).isTrue(); + + assertThat(opts.pageSize()).isEqualTo(pageSize); + assertThat(opts.pageToken()).isEqualTo(pageToken); + assertThat(opts.filter()).isEqualTo(filter); + assertThat(opts.hashCode()).isEqualTo(108027089); + } + + @Test + public void listEquality() { + Options o1; + Options o2; + Options o3; + + o1 = Options.fromListOptions(); + o2 = Options.fromListOptions(); + assertThat(o1.equals(o2)).isTrue(); + + o2 = Options.fromListOptions(Options.pageSize(1)); + assertThat(o1.equals(o2)).isFalse(); + assertThat(o2.equals(o1)).isFalse(); + + o3 = Options.fromListOptions(Options.pageSize(1)); + assertThat(o2.equals(o3)).isTrue(); + + o3 = Options.fromListOptions(Options.pageSize(2)); + assertThat(o2.equals(o3)).isFalse(); + + o2 = Options.fromListOptions(Options.pageToken("t1")); + assertThat(o1.equals(o2)).isFalse(); + + o3 = Options.fromListOptions(Options.pageToken("t1")); + assertThat(o2.equals(o3)).isTrue(); + + o3 = Options.fromListOptions(Options.pageToken("t2")); + assertThat(o2.equals(o3)).isFalse(); + + o2 = Options.fromListOptions(Options.filter("f1")); + assertThat(o1.equals(o2)).isFalse(); + + o3 = Options.fromListOptions(Options.filter("f1")); + assertThat(o2.equals(o3)).isTrue(); + + o3 = Options.fromListOptions(Options.filter("f2")); + assertThat(o2.equals(o3)).isFalse(); + } + + @Test + public void readOptTest() { + int limit = 3; + Options opts = Options.fromReadOptions(Options.limit(limit)); + + assertThat(opts.toString()).isEqualTo("limit: " + Integer.toString(limit) + " "); + assertThat(opts.hashCode()).isEqualTo(964); + } + + @Test + public void readEquality() { + Options o1; + Options o2; + Options o3; + + o1 = Options.fromReadOptions(); + o2 = Options.fromReadOptions(); + assertThat(o1.equals(o2)).isTrue(); + + o2 = Options.fromReadOptions(Options.limit(1)); + assertThat(o1.equals(o2)).isFalse(); + assertThat(o2.equals(o1)).isFalse(); + + o3 = Options.fromReadOptions(Options.limit(1)); + assertThat(o2.equals(o3)).isTrue(); + + o3 = Options.fromReadOptions(Options.limit(2)); + assertThat(o2.equals(o3)).isFalse(); + } + + @Test + public void queryOptTest() { + int chunks = 3; + Options opts = Options.fromQueryOptions(Options.prefetchChunks(chunks)); + assertThat(opts.toString()) + .isEqualTo("prefetchChunks: " + Integer.toString(chunks) + " "); + assertThat(opts.prefetchChunks()).isEqualTo(chunks); + assertThat(opts.hashCode()).isEqualTo(964); + } + + @Test + public void queryEquality() { + Options o1; + Options o2; + Options o3; + + o1 = Options.fromQueryOptions(); + o2 = Options.fromQueryOptions(); + assertThat(o1.equals(o2)).isTrue(); + + o2 = Options.fromReadOptions(Options.prefetchChunks(1)); + assertThat(o1.equals(o2)).isFalse(); + assertThat(o2.equals(o1)).isFalse(); + + o3 = Options.fromReadOptions(Options.prefetchChunks(1)); + assertThat(o2.equals(o3)).isTrue(); + + o3 = Options.fromReadOptions(Options.prefetchChunks(2)); + assertThat(o2.equals(o3)).isFalse(); } }