improvements to operation middleware #86

tpluscode · 2020-10-27T18:50:17Z

No description provided.

codecov-io · 2020-10-27T20:13:23Z

Codecov Report

Merging #86 (81227db) into master (8597063) will increase coverage by 0.45%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   94.80%   95.25%   +0.45%     
==========================================
  Files          11       11              
  Lines         250      274      +24     
==========================================
+ Hits          237      261      +24     
  Misses         13       13

Impacted Files	Coverage Δ
StoreResourceLoader.js	`94.73% <ø> (ø)`
lib/middleware/resource.js	`90.90% <85.71%> (-1.95%)`	⬇️
lib/middleware/operation.js	`100.00% <100.00%> (ø)`
middleware.js	`89.79% <100.00%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8597063...81227db. Read the comment docs.

tpluscode · 2020-10-28T08:11:07Z

examples/blog/api.ttl

+<Category> a hydra:Class ;
+  hydra:supportedOperation
+    <category#get>;
+  hydra:supportedProperty [
+    hydra:property <post>
+  ] ;
+  hydra:supportedProperty [
+    hydra:property <pinned-post>
+  ]
+.


I added Category class with those two properties to test the behavior of resources only used as objects of those properties

tpluscode · 2020-10-28T08:11:40Z

examples/blog/store-default/category%2Fcsvw.nt

@@ -0,0 +1,3 @@
+<http://localhost:9000/category/csvw> <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <http://localhost:9000/api/schema/Category> .
+
+<http://localhost:9000/category/csvw> <http://localhost:9000/api/schema/post> <http://localhost:9000/post/2> .


post/2 used only once here, will 404

tpluscode · 2020-10-28T08:12:20Z

examples/blog/store-default/category%2Frdf.nt

@@ -0,0 +1,3 @@
+<http://localhost:9000/category/rdf> <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <http://localhost:9000/api/schema/Category> .
+
+<http://localhost:9000/category/rdf> <http://localhost:9000/api/schema/post> <http://localhost:9000/post/3> .


post/3 used with two properties. Will give 405 when requested with a method supported by neither property

tpluscode · 2020-10-28T08:13:48Z

middleware.js

-    router.use(waitFor(init, () => middleware.resource))
-  }
-  router.use(waitFor(init, () => operation(api)))
+  router.use(waitFor(init, () => operation(api, middleware.resource)))


I moved the resource middleware inside the operation because the resource handler above may not set a single req.hydra.resource.

tpluscode · 2020-10-28T08:14:55Z

lib/middleware/resource.js

-    if (resources.length > 1) {
-      return next(new Error(`no unique resource found for: <${req.hydra.term.value}>`))
+      debug('Multiple resource candidates found')
+      return next()


Does not error immediately but instead set the potential "property operation" candidates to be filtered by the operation middleware

tpluscode · 2020-10-28T08:16:04Z

lib/middleware/operation.js

+      return clownface({
+        _context: [...matched._context, more._context],
+      })


Is this the correct way to merge multiple clownface contexts?

I use this way to combine all potential matching operations for the property candidates

tpluscode · 2020-10-28T08:20:36Z

lib/middleware/resource.js

+    if (resources.length > 1) {
+      return next(new Error(`no unique resource found for: <${req.hydra.term.value}>`))
+    }


This check is moved up because only "multiple class resources" are an issue.

Multiple candidates for property operation can still be reconciled

tpluscode · 2020-11-27T11:00:22Z

lib/middleware/operation.js

+  if (middleware.resource) {
+    router.use(middleware.resource)
+  }
+  router.use(invokeOperation)


I'm a little concerned about this part. The middleware.resource has to be aware that the req.hydra.resource might be undefined if no resource+operation pair was selected.

It would make sense to move it inside invokeOperation handler but because it is also an express middleware then the result might be a little awkward.

tpluscode · 2020-11-27T11:04:03Z

lib/middleware/operation.js

+    if (operationMiddleware) {
+      operations = operationMiddleware(operations, { req, res })


Calling the operations middleware hook every time. This way an API might get a chance to select an operation even if none were found otherwise. Or replace those found with something else completely.

tpluscode · 2020-11-27T11:06:11Z

lib/middleware/operation.js

+    if (operationMiddleware) {
+      operations = operationMiddleware(operations, { req, res })
+    }


Calling the operations middleware hook every time. This way an API might get a chance to select an operation even if none were found otherwise. Or replace those found with something else completely.

tpluscode · 2020-11-27T11:07:21Z

lib/middleware/operation.js

+  if (middleware.resource) {
+    router.use(middleware.resource)


I'm a little concerned about this. The middleware.resource must be aware that req.hydra.resource can be undefined if no resource+operation pair was indeed found.

Maybe move it inside invokeOperation somehow?

tpluscode · 2020-11-27T11:10:05Z

lib/middleware/operation.js

    }

+    return [{ resource, operation: null }]


Keeps the resources without any operations around to later determine if they are subject or object usage

bergos · 2020-11-28T20:55:50Z

lib/middleware/operation.js

+
+    if (operations.length > 1) {
+      error('Multiple operations found')
+      return next(new Error('Ambiguous operation'))


I'm not sure, but maybe we could use 409 Conflict?

400 range is intended for client errors. Failure to find the right operations is 99% going to be a server issue.

I think most of the time it would be a missing or unclear rdf:type in a request body. But maybe it's better that the custom middleware explicitly sets that status code for such cases.

bergos · 2020-11-28T21:02:06Z

lib/middleware/operation.js

+  const { api } = req.hydra
+
+  if (!req.hydra.operation) {
+    const operationMap = mapOperations({ api, res })


mapOperations does not call operationMiddleware, if given. For the edge case that an additional operation would be added with a different method, that method would not be included in the allowed list. I'm not sure if it needs to be covered. An additional operation would mean it was not discovered using the API description, so it's anyway somehow hidden. But it could be also the case, that we need it for inline operations, once we add that feature.

The core benefit I'm looking for right now is ability to disambiguate operations if multiple are found.

While this also allows adding/replacing operations, I would say that one does on their own responsibility

We can keep it like this now, but in a separate PR we should have a look at all the edge cases.

bergos · 2020-11-28T21:08:15Z

lib/middleware/resource.js

@@ -2,26 +2,21 @@ const { debug } = require('../log')('resource')

 function factory ({ loader }) {
  return async (req, res, next) => {
-    let resources = await loader.forClassOperation(req.hydra.term)
+    const classResources = await loader.forClassOperation(req.hydra.term)
+    const propertyObjectResources = await loader.forPropertyOperation(req.hydra.term)


The search for property operations can be more expensive on some stores/loaders. Maybe we can add an option to skip the property operation search if there is already a class operation. Just a remark for a possible future PR. This should not be covered in this PR.

Right, that's how it was before pretty much. I would probably propose a separate PR to actually combine the resource.js and operation.js seeing how the former shrunk and only really calls the loader. This way should be easier to implement this again elegantly

bergos · 2020-11-28T21:20:26Z

lib/middleware/operation.js

+    let operations = mapOperations({ api: req.hydra.api, res, method })
+      .filter(({ operation }) => operation)
+
+    if (operationMiddleware) {


The operationMiddleware is not a typical express middleware. I think that could be confusing. Last time we discussed it, I tended to use a more focused callback interface. Now that I have again a look at the code and see the existing middleware.resource, I would prefer changing it to a normal express middlware. That means wrapping it with Router to support all kinds of middlware arguments (plain function and arrays). operations could be attached to req.hydra.

bergos · 2020-11-30T16:30:58Z

lib/middleware/operation.js

+  const { api } = req.hydra
+
+  if (!req.hydra.operation) {
+    const operationMap = mapOperations({ api, res })


We can keep it like this now, but in a separate PR we should have a look at all the edge cases.

bergos · 2020-11-30T16:40:35Z

lib/middleware/operation.js

+
+    if (operations.length > 1) {
+      error('Multiple operations found')
+      return next(new Error('Ambiguous operation'))


I think most of the time it would be a missing or unclear rdf:type in a request body. But maybe it's better that the custom middleware explicitly sets that status code for such cases.

tpluscode mentioned this pull request Oct 27, 2020

Unexpected responses from property objects with no operations #87

Closed

tpluscode requested a review from bergos October 28, 2020 07:57

tpluscode commented Oct 28, 2020

View reviewed changes

tpluscode marked this pull request as ready for review November 27, 2020 10:53

tpluscode commented Nov 27, 2020

View reviewed changes

tpluscode changed the title ~~failing cases for property operations~~ improvements to operation middleware Nov 27, 2020

tpluscode force-pushed the property-operations-responses branch from a4e371b to 4cc212b Compare November 27, 2020 13:13

bergos suggested changes Nov 28, 2020

View reviewed changes

tpluscode requested a review from bergos November 30, 2020 10:25

tpluscode force-pushed the property-operations-responses branch from 2af0a95 to 81227db Compare November 30, 2020 10:35

tpluscode mentioned this pull request Nov 30, 2020

feat: middleware to disambiguate operations zazuko/cube-creator#258

Merged

tpluscode added 13 commits November 30, 2020 16:29

test: failing cases for property operation

130cfb2

fix: more appropriate responses to negative result

6b20c4e

test: update e2e scenarios

1e33e7c

refactor: remove commented out code

822470c

refactor: extract and combine

658f980

style: fix js standard

e496264

process operations paired with resources

61d834c

refactor: process operations paired with resources

1f1e83c

feat: add operations hook

28132f8

refactor: remove api param from operation middleware factory

60b4cb3

style: reorder functions to improve diff

f03041a

refactor: move resource debug messages where they makes sense

95f617e

fix: make operarions middleware proper express handler

ff22a44

tpluscode force-pushed the property-operations-responses branch from 81227db to ff22a44 Compare November 30, 2020 16:15

bergos approved these changes Nov 30, 2020

View reviewed changes

tpluscode merged commit 14cdcee into master Dec 1, 2020

tpluscode deleted the property-operations-responses branch December 1, 2020 08:42

tpluscode mentioned this pull request Jan 15, 2021

fix: missing from hydra-box 0.5 DefinitelyTyped/DefinitelyTyped#50642

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvements to operation middleware #86

improvements to operation middleware #86

tpluscode commented Oct 27, 2020

codecov-io commented Oct 27, 2020 •

edited

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Oct 28, 2020

tpluscode Nov 27, 2020

tpluscode Nov 27, 2020

tpluscode Nov 27, 2020

tpluscode Nov 27, 2020

tpluscode Nov 27, 2020

bergos Nov 28, 2020

tpluscode Nov 30, 2020

bergos Nov 30, 2020

bergos Nov 28, 2020

tpluscode Nov 30, 2020

bergos Nov 30, 2020

bergos Nov 28, 2020

tpluscode Nov 30, 2020

bergos Nov 28, 2020

bergos Nov 30, 2020

bergos Nov 30, 2020

		@@ -0,0 +1,3 @@
		<http://localhost:9000/category/csvw> <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <http://localhost:9000/api/schema/Category> .

		<http://localhost:9000/category/csvw> <http://localhost:9000/api/schema/post> <http://localhost:9000/post/2> .

		@@ -0,0 +1,3 @@
		<http://localhost:9000/category/rdf> <http://www.w3.org/1999/02/22-rdf-syntax-ns#type> <http://localhost:9000/api/schema/Category> .

		<http://localhost:9000/category/rdf> <http://localhost:9000/api/schema/post> <http://localhost:9000/post/3> .

		if (operationMiddleware) {
		operations = operationMiddleware(operations, { req, res })

improvements to operation middleware #86

improvements to operation middleware #86

Conversation

tpluscode commented Oct 27, 2020

codecov-io commented Oct 27, 2020 • edited

Codecov Report

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

codecov-io commented Oct 27, 2020 •

edited