Saved/Cached Queries #96

masojus · 2017-05-02T07:35:36Z

Fixes Issue #87 by adding support for Saved/Cached Queries.

This satisfies both the MVP and Stretch goals for that issue.
Support needed to be added in a back-compat way for parsing raw root JSON Arrays in HTTP responses, which wasn't there before. This affects all clients, including Android, so I'll be sure to test this in the sample apps.
There was no notion of Master Key usage in the Java SDK outside of Scoped Keys, so that was added. If that's not desirable, we could consider scoping the notion of the Master Key to the SavedQueries interface, but we'd want to be sure that makes sense for future work (Access Keys and Datasets, for starters) which also need Master Key for some functionality.
There was no notion of an HTTP verb other than POST before this.
This change does not invent any special representation for responses to PUT requests or the ancillary data returned from .../result endpoints, but rather relies on pre-existing QueryResult to represent the actual query result, and just a Map<String, Object> to represent everything else like the Saved Query definitions. We could look at adding some helpers if it seems frequent that client code wants to access things like run_information and properties related to create/update info.
Look at individual commit messages for more details.
I'll follow up with a README/changelog change with new versioning. I was planning to make this be v5.1.0 since it adds a big chunk of functionality.
I'll open an Issue to fix the __fake_root stuff in a better way that requires a breaking change, but in a vnext_major branch.
There are a few remaining TODOs in the code. I'm hoping to get some feedback during PR and address those, or else open Issues for them.

…o go about implementing Saved/Cached Queries and Datasets.

@Geeber

- In response to feedback from @Geeber in a previous PR. - KeenQueryTest was getting unwieldy. There's still plenty more to do to clean up existing tests, but at least get these sharing a common set of base functionality before adding new tests. - In preparation for adding tests for Saved/Cached Queries, Access Keys and Datasets.

- Previously there was no notion of a JSON response comprised of a top-level JSON Array, which we now need for API endpoints like getting all Saved Query definitions. Other endpoints return an array too, like listing Keen Projects in the organizational management APIs and listing Collection schemas. - For now, where we read JSON, so as to not change the public interface, we'll shove a top-level array into a special root key and return a Map anyway. Later we'll have to change the KeenJsonHandler interface to represent the fact that readJson() could return either a Map<> or a List<>. - This change only fixes the glitch for the JacksonJsonHandler, not the AndroidJsonHandler (or the TestJsonHandler), but I know how that needs to change as well, and we'll have to add tests for both. - Add a notion of using various HTTP request methods, since we are now accessing parts of the API that require GET/PUT/DELETE in addition to POST. - Understand request methods that don't/shouldn't have a body. HttpURLConnection will turn a GET into a POST if it sees a body and setDoOutput(true), which isn't what we want. Plus we likely didn't intend to try to GET with a payload, so express that in the helpers and KeenQueryClient. - KeenQueryRequest will be the place to get the knowledge of what HTTP method to use for a given request. We might later break KeenQueryRequest into two interfaces: one for representing the HTTP Request config options, and one for generating payload, since those are sort of separate concerns, and things are starting to get muddied. - Add some wrappers in KeenQueryClient so that code dispatching Query-related requests can request a List<> response or a Map<> response based on the the endpoint they're hitting, since at that layer of abstraction, calling code should know what to expect.

- KeenQueryRequest will again be the place where this configuration is housed. Normal transient queries are sent with a read key, so default to that.

…k in progress: - Add some new KeenQueryRequest descendants that represent these operations. PersistentAnalysis is, at a high level, meant to represent a Saved/Cached Query request or a Cached Dataset request in the future. - The SavedQueryRequest class specializes PersistentAnalysis for use with the Saved/Cached Queries API, and SavedQueryPut further overrides that behavior for Create/Update scenarios. - Add a Test class for this stuff, which isn't doing much testing and hits a real server, for now. This needs to be properly mocked and expanded upon. - Currently the operations that work are: Create a Saved Query, Create a Cached Query, Get all Saved/Cached Query definitions, Get a specific Saved/Cached Query definition, Delete a Saved/Cached Query. - Missing or only partially functioning are: Get a Saved/Cached Query result, Update a Saved/Cached Query definition. This gets at least a rudimentary result parsing setup working for Saved/Cached Queries: - Grab the result and look at the query definition to extract the hints our existing result parsing code needs. - Once massaged into the right shape, pass the result Map<> through the existing code to generate a QueryResult. - We could consider creating a new QueryResult for Saved/Cached Queries that also includes some of the other information, like 'run_information' and 'refresh_rate' and all that, though that information is availabe in the GetDefinition() return value too, so I'm not sure where/how to represent that. Maybe minimum it would be nice to expose a property bag with the *other* stuff in addition to the QueryResult? - Added a simple test that is designed to hit the real server...that won't get merged into 'master' as is though. Get Saved/Cached Query Update working. - This completes the basic CRUD operations for Saved/Cached Queries. - Plenty of work to do to polish this up and properly test it. - We still need to add some helpers to make things nice for the 3rd client devs.

- Not tested yet.

…f defining update maps in Java.

…uery API.

…y fix: - Note we now use the JSONTokener class to get the next JSON thing and then check the runtime type to see if it's JSONArray or JSONObject. - Technically this is a breaking change if any client codes implements the AndroidJsonHandler.JsonObjectManager interface since they'd have to add the newTokener() override--I'm willing to bet this isn't super common, though. If we think for some reason it *is* common, we could instead cast to AndroidJsonObjectManager and if not of that runtime type, bail in some other way that will still succeed in real code but maybe fails in test code since we can't easily mock it? That or add a completely new interface that is optionally set... - Two tests verify that both a simple JSON Object and a simple JSON Array can get parsed into Map<> and List<> respectively.

- Update is still untested. - GETting a result(s) is still untested.

masojus · 2017-05-02T21:43:08Z

query/src/main/java/io/keen/client/java/KeenQueryClient.java

+        Object response = getResponse(request);
+
+        if (!containerType.isAssignableFrom(response.getClass())) {
+            throw new RuntimeException(); // TODO : More specific


Deal with this TODO before merge.

masojus · 2017-05-02T21:44:52Z

query/src/main/java/io/keen/client/java/KeenQueryClient.java

+    }
+
+    Map<String, Object> getMapResponse(KeenQueryRequest request) throws IOException {
+        // Throw are runtime if the return type isn't as expected.


Deal with this TODO and same one below before merge.

masojus · 2017-05-03T04:00:04Z

query/src/main/java/io/keen/client/java/KeenQueryClient.java

+
+    private QueryResult rawMapResponseToQueryResult(KeenQueryRequest request,
+                                                    Map<String, Object> response) {
+        // TODO : Eventually moving result parsing out of this class might be a good plan, like


Probably should just create an issue for this one, since it's going to be a large refactor.

masojus · 2017-05-03T04:24:46Z

query/src/test/java/io/keen/client/java/KeenQueryTestBase.java

+        List<Response> remaining = responses.subList(1, numExecuteCalls);
+        Response[] responsesArray = remaining.toArray(new Response[numExecuteCalls - 1]);
+
+        Response r = responses.get(0);


Could clean this up.

baumatron

Massive change, so hopefully I didn't miss too much! Looks like lots of goodness 🏅

baumatron · 2017-05-03T22:40:56Z

core/src/main/java/io/keen/client/java/KeenClient.java

+                    ((KeenDetailedCallback)callback).onSuccess(project,
+                                                               eventCollection,
+                                                               event,
+                                                               keenProperties);
                }
            } catch (Exception userException) {
                // Do nothing.


I realize it isn't a part of this change, but I'm curious why we're silently catching these exceptions and if there's any history there, or if there is a platform-specific reason for doing this. Shouldn't we allow them to be propagate? Presumably an exception unhandled by a client really should take down the app, right?

This seems like a good question, especially because I think this is calling client code (the success callback). If that errors, the user should know about it.

Not required to fix in this PR, if you don't want to.

Bummer, I typed up an entire response to this and apparently never clicked submit. Anyway the idea is a telemetry module should not take down your app since it's not crucial for event reporting to work in order for the app to work. So, failures are explicitly handled in a callback, or else they're swallowed. I think for event write, this is the proper technique, but for queries I would not do this, because one is asking for data that presumably is needed to show the user something, so failure isn't a tolerable scenario. Especially, in this case, we shouldn't re-throw because it's in the callback to client code. If client code wants to throw an exception in their callback, well, OK, I guess we could opt to never prevent you from shooting yourself in the foot, but I'd say this is a better solution in an event reporting SDK. That said, we should probably log it, so I'll make a note to add logging to these.

Though @josephwegner makes a good point...if the exception is raised in client code, then perhaps we really should re-throw that. I guess I could argue either side of this one. I'll file an issue and we'll hash it out there.

Created Issue #98.

baumatron · 2017-05-04T00:37:03Z

core/src/main/java/io/keen/client/java/KeenClient.java

+                               String eventCollection,
+                               Map<String, Object> event,
+                               Map<String, Object> keenProperties) {
+        handleSuccess(callback);
        if (callback != null) {
            try {
                if(callback instanceof KeenDetailedCallback){


Nit: while you're here, there's a space missing after if.

baumatron · 2017-05-04T00:38:58Z

core/src/main/java/io/keen/client/java/KeenProject.java

+     *                  functionality that requires such a key.
+     */
+    public KeenProject(String projectId, String writeKey, String readKey, String masterKey) {
+        if (projectId == null || projectId.trim().isEmpty()) {


Do we care to do any sort of key validation here? Is that done elsewhere already?

What sort of validation? Since this is tossed about between modules, we can't yet know if a master is needed or a write or a read, because you may only be doing queries, or only doing administrative operations, or only doing writes, or whatever, so in theory we can't know what's needed. That said, at least one of them should be non-null and non-empty, so at a minimum we could validate that.

baumatron · 2017-05-04T00:52:12Z

java/src/main/java/io/keen/client/java/JacksonJsonHandler.java

-        return mapper.readValue(reader, MAP_TYPE);
+        // TODO : We can't assume the top-level node is a JSON Object anymore, because parts of the
+        // API we access return a JSON Array as the root. So we need to detect the type and decide
+        // what to return, so we need a different return type. Technically it could be a List or


Wording here is confusing: "...so we need..."

Yeah I'll replace this TODO with either a clearer NOTE or just create an Issue, but do you have opinions on how to handle the situation in the next major revision? I'd be happy to hear what layer of abstraction you think should handle the branch between top-level object and top-level array.

baumatron · 2017-05-04T16:56:51Z

core/src/main/java/io/keen/client/java/http/HttpMethods.java

+ *
+ * @author masojus
+ */
+public final class HttpMethods {


baumatron · 2017-05-04T18:58:44Z

query/src/main/java/io/keen/client/java/SavedQueriesImpl.java

+        // We expect a structure such that each entry in the list was a JSON Object representing a
+        // query definition, and no entry should be a JSON Value.
+        for (Object defObj : response) {
+            if (!(defObj instanceof LinkedHashMap)) {


What about using something less specific here like Map?

I was intentionally more specific only because I happen to know empirically that LinkedHashMap is what that lib should produce. But, you're right in that we could be more lenient and future-proof if we accepted the most abstract interface that provides the necessary functionality, so I'll change it. Thanks!

baumatron · 2017-05-04T19:54:49Z

query/src/main/java/io/keen/client/java/SavedQueryPut.java

+
+            requestArgs.put(KeenQueryConstants.QUERY, queryArgs);
+
+            // TODO : ... or does it go here? Seems like it works both ways :S


Any resolution here?

None that I've heard.

@josephwegner Confirmed that it should go on the query object as the docs state.

baumatron · 2017-05-04T20:15:56Z

query/src/test/java/io/keen/client/java/PersistentAnalysisTest.java

+    }
+
+    // Make sure result, if provided, is wrapped in root object/array brackets, if not scalar.
+    private String getFakeSavedQueryDefinition(String result, boolean isGroupBy, boolean isInterval, boolean isMultiAnalysis, boolean isFunnel) {


Nit: long line.

baumatron · 2017-05-04T20:19:14Z

query/src/test/java/io/keen/client/java/PersistentAnalysisTest.java

+            analysisType = "sum";
+        }
+
+        String definition = "{" +


Oh man... no here string literal in Java? 😞

baumatron · 2017-05-04T20:21:10Z

query/src/test/java/io/keen/client/java/PersistentAnalysisTest.java

+        // Should have 'refresh_rate' and 'query' top-level keys, at least.
+        assertTrue("Missing required top-level fields.", 2 <= requestNode.size());
+
+        assertTrue(requestNode.hasNonNull(KeenQueryConstants.REFRESH_RATE));


Just to confirm, refresh_rate must be specified because if it isn't the API/backend will set it to the minimum cached query interval, right?

Yes, for now. It seems like a bug, but I'm not sure when they'll get to fixing it, and it's just as easy for us to pass it along.

josephwegner · 2017-05-11T19:29:38Z

android/src/main/java/io/keen/client/android/AndroidJsonHandler.java

+            // TODO : What should the return value be? Technically it could be a List or Map, so it
+            // should be Object, then client code would need to do the instanceof check. For now,
+            // so as to not break the KeenJsonHandler interface, we can stick a dummy "root" key in
+            // the map we pass back.


Is there anywhere in the code where this isn't passed a map? I believe our API will always respond in a map format.

Either way, I think I'm fine with creating a fake root object so it becomes a map... but I'm not terribly opinionated on the matter.

The problem is that the JSON parser will return either a Map<> or a List<> depending on the root JSON structure. Usually the root is a JSON Object, but now we have to handle a JSON Array as a root object too. So in the vnext_major branch we should choose whether to branch here or at a higher layer of abstraction. This TODO is mostly an inquiry to you folks in case you have an opinion as to where that branching should occur, but it doesn't have to be decided today...and if you have no opinion then I'll just do what I think is best in the vnext_major branch and leave this as is for the minor release.

josephwegner · 2017-05-11T19:33:46Z

core/src/main/java/io/keen/client/java/KeenClient.java

+                    ((KeenDetailedCallback)callback).onSuccess(project,
+                                                               eventCollection,
+                                                               event,
+                                                               keenProperties);
                }
            } catch (Exception userException) {
                // Do nothing.


This seems like a good question, especially because I think this is calling client code (the success callback). If that errors, the user should know about it.

Not required to fix in this PR, if you don't want to.

josephwegner · 2017-05-11T19:36:08Z

core/src/test/java/io/keen/client/java/TestKeenClientBuilder.java

    private final Environment environment;
-    private boolean isNetworkConnected = true;


Why remove this?

I think it just wasn't used, but I'll make sure it has no effect.

josephwegner · 2017-05-11T19:39:57Z

query/src/main/java/io/keen/client/java/PersistentAnalysis.java

+    // display name of "Can has diacritics Ñüáúéíó üöäñ çàèìòù ãõ" yields a url as follows:
+    // "https://...?saved_query=can-has-diacritics----". So two questions remain:
+    // 1) Can the API handle a queryName with non-ASCII? Should we allow ?
+    // 2) This regex does not match strings like "árbol π" so we only accept ASCII


I think disallow, since the API filters them out.

Ok that's what it does. I'll remove the comment or adjust it to reflect what and why.

josephwegner · 2017-05-11T19:43:02Z

query/src/main/java/io/keen/client/java/RefreshRate.java

+
+    // The refresh rate range empirically is [14400, 86400] seconds, inclusive at both boundaries.
+    // A refresh rate of 0 means caching is turned off.
+    // TODO : Docs on the website are wrong as of 3/14/17, as has been reported. Get that fixed.


Can we remove this TODO? The related changes won't be in this codebase, and either what is wrong is unclear. I have a feeling this will exist forever if we commit it.

Yeah this TODO will go away no matter what before merging. It's just so you and other reviewers see it and so it's in the git history, because it needs to be fixed but is external to the SDK.

josephwegner · 2017-05-11T19:44:44Z

query/src/main/java/io/keen/client/java/RefreshRate.java

+    public static final int MAX = 86400; // Maximum is 24 hrs
+
+    public static int fromHours(int hours) {
+        int refreshRate = hours * 3600;


nit: can we make a constant called HOUR_IN_SECONDS, that contains 3600?

Yep will do.

josephwegner · 2017-05-11T19:45:51Z

query/src/main/java/io/keen/client/java/RefreshRate.java

+    }
+
+    static void validateRefreshRate(int refreshRate) {
+        if (0 != refreshRate && (RefreshRate.MIN > refreshRate || RefreshRate.MAX < refreshRate)) {


Also, NO_CACHING isn't invalid is it? It just means... no caching?

josephwegner · 2017-05-11T19:49:03Z

query/src/main/java/io/keen/client/java/SavedQueryPut.java

+                  KeenQueryRequest query,
+                  int refreshRate,
+                  Map<String, ?> miscProperties) {
+        super(HttpMethods.PUT, true /* needsMasterKey */, queryName, displayName);


Why the comment here?

I hate literals as actual parameters when reading code if not in the IDE. In the IDE I can just hover and see what formal parameter that maps to, but reading it elsewhere, like here in GitHub, it's so much more useful to have the comment for null and boolean literals and number type literals, etc. It's something I always do in C and C++ programming, and if there's a good reason not to do it in Java, let me know, but my opinion is that it adds clarity.

Sometimes I like to use enums for that reason, just another option. Sometimes that's overkill though. Named parameters would be nice.

For sure. In .NET or Python I'd just lean on named params. For null and arbitrary numerics an enum doesn't always help though. I like to either name a var or put the comment there.

masojus · 2017-05-12T23:39:41Z

java/src/main/java/io/keen/client/java/JacksonJsonHandler.java

+        Map<String, Object> rootMap = null;
+
+        /*
+        TODO : Are the calls to traverse() here any better or worse than doing either of these?:


If nobody has any insight here, I'll just leave it as is and remove the comment.

- Note that the change to throw in the KeenProject ctor could arguably be considered a breaking change. However, I don't see a realistic use case where creating a KeenProject with *just* a Project ID is useful. You can create Scoped Keys with a blank KeenClient, but you need not set its KeenProject at all, much less with just a Project ID, so I don't think this merits a major version bump.

masojus · 2017-05-17T08:35:07Z

I think I've addressed all feedback, either via code changes or by responding here inline or opening an Issue. If this all looks OK, I'll get it merged, and then I'll follow up with a PR that updates the Readme/Changelog and bumps the version to 5.2.0 in preparation for releasing the Maven artifacts.

masojus added 12 commits April 23, 2017 01:53

A few comment/whitespace/code style changes as I looked through how t…

e675e84

…o go about implementing Saved/Cached Queries and Datasets.

Fix some warnings in the 'query' module tests and fill in some comments.

0d1a9d2

Add the concept of a Master Key.

5d2774c

- KeenQueryRequest will again be the place where this configuration is housed. Normal transient queries are sent with a read key, so default to that.

Handle root JSON array response bodies in the AndroidJsonHandler.

693d740

- Not tested yet.

Fix up some comments and add a few helpers to assuage the verbosity o…

86d051d

…f defining update maps in Java.

Fix some TODO items around validation of inputs to the Saved/Cached Q…

0a256f2

…uery API.

Start adding tests.

e274c53

- Update is still untested. - GETting a result(s) is still untested.

Expand on tests.

e560ddc

masojus requested review from josephwegner, Geeber and baumatron May 2, 2017 07:35

masojus commented May 3, 2017

View reviewed changes

masojus commented May 4, 2017

View reviewed changes

baumatron approved these changes May 4, 2017

View reviewed changes

josephwegner reviewed May 11, 2017

View reviewed changes

masojus commented May 12, 2017

View reviewed changes

This was referenced May 17, 2017

Deal with __fake_root #99

Open

KeenQueryTestBase lower-level HTTP Request validation #100

Open

ServerException vs KeenQueryClientException #101

Open

baumatron approved these changes May 18, 2017

View reviewed changes

masojus merged commit d2e3089 into master May 18, 2017

masojus mentioned this pull request May 18, 2017

Add support for Cached Queries #87

Closed

7 tasks


		requestArgs.put(KeenQueryConstants.QUERY, queryArgs);

		// TODO : ... or does it go here? Seems like it works both ways :S

		private final Environment environment;
		private boolean isNetworkConnected = true;

Saved/Cached Queries #96

Saved/Cached Queries #96

Conversation

masojus commented May 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baumatron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masojus May 12, 2017 • edited Loading

Choose a reason for hiding this comment

masojus May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masojus May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masojus May 18, 2017 • edited Loading

Choose a reason for hiding this comment

masojus May 12, 2017 • edited Loading

Choose a reason for hiding this comment

masojus commented May 17, 2017 • edited Loading

masojus commented May 2, 2017 •

edited

Loading

masojus May 12, 2017 •

edited

Loading

masojus May 12, 2017 •

edited

Loading

masojus May 12, 2017 •

edited

Loading

masojus May 18, 2017 •

edited

Loading

masojus May 12, 2017 •

edited

Loading

masojus commented May 17, 2017 •

edited

Loading