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.

Sign up for GitHub

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

Jump to bottom

Fix "surprising queries" samples. #242

Merged
merged 2 commits into from
May 18, 2016
Merged

Fix "surprising queries" samples. #242

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e49fed4
Fix "surprising queries" samples.
tswast May 17, 2016
0f11a9f
Add comments pointing to documentation for surprising query results.
tswast May 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 36 additions & 27 deletions appengine/datastore/src/test/java/com/example/appengine/QueriesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@

import java.io.PrintWriter;
import java.io.StringWriter;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

Expand Down Expand Up @@ -685,24 +684,24 @@ public void queryRestrictions_sortWrongOrderOnInequality_isInvalid() throws Exce
public void queryRestrictions_surprisingMultipleValuesAllMustMatch_returnsNoEntities()
throws Exception {
Entity a = new Entity("Widget", "a");
ArrayList<Long> xs = new ArrayList<>();
xs.add(1L);
xs.add(2L);
List<Long> xs = Arrays.asList(1L, 2L);
a.setProperty("x", xs);
datastore.put(a);

// [START surprising_behavior_example_1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine as is, though a shorter alternative could be:
List<Long> xs = Arrays.asList(1L, 2L);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Query q =
new Query("Widget")
.setFilter(new FilterPredicate("x", FilterOperator.GREATER_THAN, 1))
.setFilter(new FilterPredicate("x", FilterOperator.LESS_THAN, 2));
.setFilter(
CompositeFilterOperator.and(
new FilterPredicate("x", FilterOperator.GREATER_THAN, 1),
new FilterPredicate("x", FilterOperator.LESS_THAN, 2)));
// [END surprising_behavior_example_1]

// Note: The documentation describes that the entity "a" will not match
// because no value matches all filters. When run with the local test
// runner, the entity "a" *is* matched. This may be a difference in
// behavior between the local devserver and Cloud Datastore, so there
// aren't any assertions we can make in this test.
// Entity "a" will not match because no individual value matches all filters.
// See the documentation for more details:
// https://cloud.google.com/appengine/docs/java/datastore/query-restrictions#properties_with_multiple_values_can_behave_in_surprising_ways
List<Entity> results = datastore.prepare(q).asList(FetchOptions.Builder.withDefaults());
assertThat(results).named("query results").isEmpty();
}

@Test
Expand All @@ -716,21 +715,24 @@ public void queryRestrictions_surprisingMultipleValuesEquals_returnsMatchedEntit
c.setProperty("x", ImmutableList.<Long>of(-6L, 2L));
Entity d = new Entity("Widget", "d");
d.setProperty("x", ImmutableList.<Long>of(-6L, 4L));
datastore.put(ImmutableList.<Entity>of(a, b, c, d));
Entity e = new Entity("Widget", "e");
e.setProperty("x", ImmutableList.<Long>of(1L, 2L, 3L));
datastore.put(ImmutableList.<Entity>of(a, b, c, d, e));

// [START surprising_behavior_example_2]
Query q =
new Query("Widget")
.setFilter(new FilterPredicate("x", FilterOperator.EQUAL, 1))
.setFilter(new FilterPredicate("x", FilterOperator.EQUAL, 2));
.setFilter(
CompositeFilterOperator.and(
new FilterPredicate("x", FilterOperator.EQUAL, 1),
new FilterPredicate("x", FilterOperator.EQUAL, 2)));
// [END surprising_behavior_example_2]

// Only "a" and "e" have both 1 and 2 in the "x" array-valued property.
// See the documentation for more details:
// https://cloud.google.com/appengine/docs/java/datastore/query-restrictions#properties_with_multiple_values_can_behave_in_surprising_ways
List<Entity> results = datastore.prepare(q).asList(FetchOptions.Builder.withDefaults());
assertThat(getKeys(results)).named("query result keys").contains(a.getKey());

// Note: When run in the test server, this matches "c" as expected and does
// not match "d" as expected. For some reason it does *not* match "b".
// The behavior of queries on repeated values is definitely surprising.
assertThat(getKeys(results)).named("query result keys").containsExactly(a.getKey(), e.getKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you are not using the Datastore Query.setKeysOnly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the results will still be a list of Entities in that case. I want to do an equality check on the keys rather than the entity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why/how a key-only query return you Entities and not Keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PreparedQuery always returns Entity. https://cloud.google.com/appengine/docs/java/javadoc/com/google/appengine/api/datastore/PreparedQuery#asList-com.google.appengine.api.datastore.FetchOptions-

I don't see any method in the PreparedQuery class that returns a iterable of Key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but if you set the setKeysOnly on the query the returned Entities will only contain Keys (and could be compared as is). I think in gcloud-java-datastore we did a better job and return a List<Key> rather than a List<Entity> which only holds the Keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I still need the getKeys() method. It's running against a local datastore and only getting like 3 entities, so adding setKeysOnly() doesn't really do much for me in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually because you are not using setKeysOnly couldn't you have done:
assertThat(results).named("query result").containsExactly(a, e); ?

Because this is part of user interfacing samples (I assume that even the tests are meant to be used as reference) I would highly recommend using the ideal code for accomplishing what is needed (regardless if
that actually does not make any difference in small scale or for testing). Feel free to ignore if tests are not
meant for user consumption or if you think otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. .equals() only checks the key, so that will have the same effect. I'll send a follow-up PR for this change. I do want the tests to be as easy to understand as possible.

https://cloud.google.com/appengine/docs/java/javadoc/com/google/appengine/api/datastore/Entity#equals-java.lang.Object-

}

@Test
Expand All @@ -752,6 +754,9 @@ public void queryRestrictions_surprisingMultipleValuesNotEquals_returnsMatchedEn
Query q = new Query("Widget").setFilter(new FilterPredicate("x", FilterOperator.NOT_EQUAL, 1));
// [END surprising_behavior_example_3]

// The query matches any entity that has a some value other than 1. Only
// entity "e" is not matched. See the documentation for more details:
// https://cloud.google.com/appengine/docs/java/datastore/query-restrictions#properties_with_multiple_values_can_behave_in_surprising_ways
List<Entity> results = datastore.prepare(q).asList(FetchOptions.Builder.withDefaults());
assertThat(getKeys(results))
.named("query result keys")
Expand All @@ -770,17 +775,21 @@ public void queryRestrictions_surprisingMultipleValuesTwoNotEquals_returnsMatche
// [START surprising_behavior_example_4]
Query q =
new Query("Widget")
.setFilter(new FilterPredicate("x", FilterOperator.NOT_EQUAL, 1))
.setFilter(new FilterPredicate("x", FilterOperator.NOT_EQUAL, 2));
.setFilter(
CompositeFilterOperator.and(
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a strange one that seems to ignore the comment in the Datastore queries doc saying that "A query can have no more than one NOT_EQUAL filter, and a query that has one cannot have any other inequality filters". @pcostell suggests that this is because the AE Datastore client breaks it
to separate queries and then join the combined results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a link to the docs.
https://cloud.google.com/appengine/docs/java/datastore/query-restrictions#properties_with_multiple_values_can_behave_in_surprising_ways

That link describes that the query acts like

x < 1 OR (x > 1 AND x < 2) OR x > 2

new FilterPredicate("x", FilterOperator.NOT_EQUAL, 1),
new FilterPredicate("x", FilterOperator.NOT_EQUAL, 2)));
// [END surprising_behavior_example_4]

// The two NOT_EQUAL filters in the query become like the combination of queries:
// x < 1 OR (x > 1 AND x < 2) OR x > 2
//
// Only "b" has some value which matches the "x > 2" portion of this query.
//
// See the documentation for more details:
// https://cloud.google.com/appengine/docs/java/datastore/query-restrictions#properties_with_multiple_values_can_behave_in_surprising_ways
List<Entity> results = datastore.prepare(q).asList(FetchOptions.Builder.withDefaults());
assertThat(getKeys(results)).named("query result keys").contains(b.getKey());

// Note: The documentation describes that the entity "a" will not match.
// When run with the local test runner, the entity "a" *is* matched. This
// may be a difference in behavior between the local devserver and Cloud
// Datastore.
assertThat(getKeys(results)).named("query result keys").containsExactly(b.getKey());
}

private Entity retrievePersonWithLastName(String targetLastName) {
Expand Down