add stored query update (PUT) method #189

brianru · 2019-05-02T17:29:17Z

depends on this Stardog PR https://github.com/stardog-union/stardog/pull/7217 (merged and released)

brianru · 2019-05-08T15:23:12Z

The stardog PR was merged.

brianru · 2019-05-08T15:58:08Z

The stored query update tests pass with a build of Stardog off develop, but other tests fail b/c they're dependent on #182 being merged.

jmrog · 2019-05-08T18:43:12Z

@brianru The stored query update tests are actually failing on CI. Can we add a version check or something like that (as in #182) so that they don't fail on earlier Stardog versions (e.g., they should expect a failure response from the API)?

brianru · 2019-05-08T18:48:19Z

@jmrog yep. will do.

brianru · 2019-05-16T15:16:07Z

@jmrog tests are failing on master on versions of stardog < 6.1.4 bc we only merged #191 to v2. what should we do here? why not merge it into master?

jmrog

This looks good to me, but I'm not sure that I really like the fact that updateStoredQuery will make additional requests when a stardogVersion isn't supplied. To me, that seems like an unexpected side effect that people might perceive as a bug of some kind (and, in order to avoid that side effect, it imposes on all future users of stardog.js a requirement to provide a version string). Could we not just go ahead and make the new update request, and, if that fails, fall back to the delete+add flow? We could then let people override this when necessary (and go immediately to the delete+add flow) by specifying an optional boolean argument rather than a Stardog version, which they'd otherwise never have to specify going forward, e.g.:

const updateStoredQuery = (conn, storedQuery, params, useUpdate = true) => {
  if (!useUpdate) {
    // old add+delete flow
  }
  else {
    // try update, then try add+delete
  }
};

One potential downside to the above is that people using Stardog < 6.2.0 will have an extra update request that fails behind the scenes. But I think that's okay for a couple of reasons: 1) it encourages upgrading, and I think it's probably good for stardog.js to most closely track the current Stardog (and/or work best with current versions), 2) it has a very easy escape hatch that lets people avoid the behavior if it's really an issue, and 3) it automatically works in the best way possible without any additional requests for users of the current and future Stardog versions, so it will ultimately be making fewer requests, especially for up-to-date Stardog users (whereas the current way will always make a version request, indefinitely into the future, unless all users take the escape hatch of providing a version number -- in which case, the escape hatch we provide in my own suggestion is essentially the same).

FWIW, we've imposed minimum version requirements for stardog.js before (it's only supported for version 5+) so it's also open to us to release a version with a new compatibility notice (but I'd prefer doing that only for major Stardog versions, not point releases). We probably should at least note that the method performs best with Stardog 6.2.0+.

Any thoughts on that @brianru or @neptunian ?

neptunian · 2019-05-20T17:14:36Z

That sounds good to me. It'd be nice not having to change our action creators for versions or keep track of them here.

I also wouldn't mind just letting the user figure it out by adding the compatible Stardog version in our docs kind of like how nodejs docs have a version for each method:

jmrog · 2019-05-21T11:33:18Z

I also wouldn't mind just letting the user figure it out by adding the compatible Stardog version in our docs kind of like how nodejs docs have a version for each method:

This would be okay here, but probably not great. The reason is that the version number node uses is the version of node itself. Here, it would actually have to be the version of Stardog that the method works with (e.g., 6.2.0+) rather than the version of stardog.js. It doesn't seem unlikely that that would confuse some people. It would also mean that people could get a version of stardog.js with methods available that simply don't work for them, which is another difference to the node case (where, if you don't have that node version, the method simply doesn't exist).

brianru · 2019-05-22T13:04:55Z

@jmrog I'm good with your approach. In summary:

optional param to useUpdate method (defaults to true) with doc explaining it's available starting 6.2.0
when set to false or when update request returns a 404/5 (have to check what stardog returns) then it'll do the delete/add

…error

brianru · 2019-05-23T12:32:12Z

@jmrog ready for review again!

neptunian · 2019-05-24T14:28:28Z

lib/index.d.ts

                */
-            function update(conn: Connection, config: StoredQueryOptions, params?: object): Promise<HTTP.Body>
+                function update(conn: Connection, config: StoredQueryOptions, params?: object, useUpdateMethod?: boolean): Promise<HTTP.Body>


spacing for update function seems off

The comment also says Default: false when the default is actually true.

Nice catch, fixed.

jmrog

Just a couple of comments.

jmrog · 2019-05-24T14:49:53Z

lib/index.d.ts

                */
-            function update(conn: Connection, config: StoredQueryOptions, params?: object): Promise<HTTP.Body>
+                function update(conn: Connection, config: StoredQueryOptions, params?: object, useUpdateMethod?: boolean): Promise<HTTP.Body>


The comment also says Default: false when the default is actually true.

lib/query/stored.js

jmrog · 2019-05-24T14:54:41Z

lib/query/stored.js

+const replace = (conn, storedQuery, params) =>
+  deleteStoredQuery(conn, storedQuery.name).then(deleteResponse => {
+    //  Update creates when the query did not already exist
+    if (deleteResponse.status === 404 || deleteResponse.ok) {


Is this check necessary? It seems like we create on both failure and success, so maybe we don't need to check.

I wanted to distinguish between 404 failures and non-404 failures. If it's a 404 I assume it means the query didn't exist and we can continue. Otherwise who knows what's happening.

That makes sense. 👍

jmrog · 2019-05-24T14:57:10Z

lib/response-transforms.js

-          response.body = null;
-        }
+      } else if (res.text) {
+        // 204 responses have `body: null` and no `text` method


Is this comment right? Can you please double-check? A fetch response should always have a .text method; if it doesn't, there's likely a bug somewhere (e.g., the fetch response itself isn't actually being passed in here, but something else is). (Relevant docs here: https://developer.mozilla.org/en-US/docs/Web/API/Response)

I was definitely seeing this behavior the other day..I had to make this change for the tests to pass. But of course I'm not seeing that behavior now. Double checking with stardog 6.2.1 vs 6.1.3 (I was switching between the 2 at the time).

Definitely getting this error with stardog 6.1.3.

Digging in some more.

Oh, it's because we've picked certain fields from the previous response isn't it..

Yeah, I have to reorder the promise thens. Fix will be up shortly.

brianru · 2019-05-28T17:29:20Z

back to you @jmrog !

reminder:

The stored query update tests pass with a build of Stardog off develop, but other tests fail b/c they're dependent on #182 being merged.

why didn't we want to merge #182 into master? the breaking change?

jmrog · 2019-06-13T14:09:55Z

@brianru If it's okay with you, I'm going to merge this into the v2 branch for now rather than master. Studio can consume it as usual. I don't want to merge it into master yet because #182 also isn't merged into master yet (yeah, because of the breaking API change). My preference is to merge both of those into master when I finish the work I've been doing on #159, #160, #161 (which will complete the Epic, #194). That way, there won't be two breaking API changes in very close succession.

brianru · 2019-06-13T14:13:15Z

sounds good @jmrog !

jmrog · 2019-06-13T14:13:58Z

sounds good @jmrog !

Great, I'll do it momentarily.

jmrog · 2019-06-13T15:21:09Z

Rebased this on v2 and merged it in. 👍

add stored query update (PUT) method

f0892cc

brianru mentioned this pull request May 2, 2019

add stored query update method #190

Closed

brianru marked this pull request as ready for review May 8, 2019 15:22

brianru mentioned this pull request May 8, 2019

json query explain on v2 branch #191

Merged

brianru added the in progress label May 9, 2019

brianru self-assigned this May 9, 2019

brianru added 2 commits May 16, 2019 09:52

handle stored query updates when version < 6.2.0

8aac492

cleanup poc of server version helper

e9df10d

jmrog reviewed May 20, 2019

View reviewed changes

brianru added 4 commits May 22, 2019 13:58

instead of getting the version recover from the method not supported …

0e58442

…error

cleanup

caa5cca

get masters package-lock.json

1970c60

cleanup

476dbc5

neptunian reviewed May 24, 2019

View reviewed changes

jmrog suggested changes May 24, 2019

View reviewed changes

code review feedback

45c8c4e

brianru added needsReview and removed in progress labels May 28, 2019

jmrog approved these changes Jun 13, 2019

View reviewed changes

jmrog closed this Jun 13, 2019

anneeb deleted the stored-query-update branch May 24, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add stored query update (PUT) method #189

add stored query update (PUT) method #189

brianru commented May 2, 2019 •

edited

Loading

brianru commented May 8, 2019

brianru commented May 8, 2019

jmrog commented May 8, 2019

brianru commented May 8, 2019

brianru commented May 16, 2019

jmrog left a comment •

edited

Loading

neptunian commented May 20, 2019

jmrog commented May 21, 2019 •

edited

Loading

brianru commented May 22, 2019

brianru commented May 23, 2019

neptunian May 24, 2019

jmrog May 24, 2019

brianru May 24, 2019

jmrog left a comment

jmrog May 24, 2019

jmrog May 24, 2019

brianru May 24, 2019

jmrog May 24, 2019

jmrog May 24, 2019 •

edited

Loading

brianru May 24, 2019

brianru May 24, 2019

brianru May 24, 2019

brianru May 24, 2019

brianru commented May 28, 2019

jmrog commented Jun 13, 2019

brianru commented Jun 13, 2019

jmrog commented Jun 13, 2019

jmrog commented Jun 13, 2019

add stored query update (PUT) method #189

add stored query update (PUT) method #189

Conversation

brianru commented May 2, 2019 • edited Loading

brianru commented May 8, 2019

brianru commented May 8, 2019

jmrog commented May 8, 2019

brianru commented May 8, 2019

brianru commented May 16, 2019

jmrog left a comment • edited Loading

Choose a reason for hiding this comment

neptunian commented May 20, 2019

jmrog commented May 21, 2019 • edited Loading

brianru commented May 22, 2019

brianru commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrog left a 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

jmrog May 24, 2019 • 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

brianru commented May 28, 2019

jmrog commented Jun 13, 2019

brianru commented Jun 13, 2019

jmrog commented Jun 13, 2019

jmrog commented Jun 13, 2019

brianru commented May 2, 2019 •

edited

Loading

jmrog left a comment •

edited

Loading

jmrog commented May 21, 2019 •

edited

Loading

jmrog May 24, 2019 •

edited

Loading