feat: access-api handling store/info for space not in db returns failure with name #391

gobengo · 2023-01-24T00:50:48Z

Motivation:

implement access-api should respond to space/info invocations for space dids not in its db with a well-defined error result type #382
- tl;dr space/info now has explicit failure type for case where the ucans are valid, but the space DID is not in the db. that way when upload-api invokes space/info as part of its verifyInvocation logic, it can distinguish between an invalid invocation and a not-found space id

Unblocks:

this test passing Add test for space/info delegation gobengo/w3protocol-test#1
rest of proposal: store/add handler should have more helpful error when trying to invoke against space that has not had email confirmed w3infra#134 (comment)

…th status=404

hugomrdias · 2023-01-24T14:04:11Z

packages/access-api/src/service/index.js

+            error: true,
+            status: 404,
+            message: 'Space not found.',
+          }


we probably should start a error.js file like this https://github.com/web3-storage/ucanto/blob/main/packages/validator/src/error.js and a error.ts for error types in the client

this is gonna return a http 200 so i dont think we need the status in there, just a normal SpaceNotFound extends Failure with name and message props.

this is gonna return a http 200

this service could be invoked via non-http transport

The status is in there as a reliable machine-readable indication of what kind of error happened. I don't think we should use just a specific 'message' value as the only discriminant of this error case because doing that would make it so that changing the error message string would be a backward-incompatible change from the perspective of any client trying to detect this error case. I'm intentionally trying to add a specific non-message error code that clients can use to detect this case.

exactly, non-http transport should not need to understand http codes. For errors IMO either use the name as the identifier or a code nodejs style error.code = ERR_SPACE_NOT_FOUND

// similar to ucanto if(error.name === 'SpaceNotFound'){} // similar to nodejs class SpaceNotFoundError{} SpaceNotFoundError.code = 'ERR_SPACE_NOT_FOUND' if(error.code === SpaceNotFoundError.code) {}

@hugomrdias Thanks, when I first responded I missed that you were encouraging a name and thought you were just saying to cut status which would make it the same as generic Failure. my bad

I updated [here] to not have a status at all and just a well-defined name 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

hugomrdias · 2023-01-24T14:07:57Z

packages/access-client/src/types.ts

+export type SpaceNotFoundFailure = Failure & {
+  status: 404
+}
+


maybe create a new errors.ts file exported from types.ts with something like https://github.com/web3-storage/ucanto/blob/main/packages/interface/src/capability.ts#L413-L416

I moved the failure type to an errors.ts file as you suggested 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

packages/access-client/src/types.ts

Gozala

Provided feedback. I don't think you need another review from me.

Gozala · 2023-01-24T21:50:07Z

packages/access-client/src/types.ts

+ * Indicates failure executing ability that requires access to a space that cannot be found by the handler
+ */
+export type SpaceNotFoundFailure = Failure & {
+  status: 404


Suggested change

status: 404

name: 'UnknownSpaceDID`

I would say ☝️, which is I think more accurate than not found. If we want to be future compatible I would go as far as do this instead.

Suggested change

status: 404

name: 'NoStorageProvider'

For what it's worth I don't mind status along with the name to categorize concrete errors into a groups with HTTP semantics, I just think it's not replacement for concrete errors.

I got rid of status and used name of UnknownSpace in 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

Gozala · 2023-01-24T21:54:30Z

packages/access-api/src/service/index.js

+          return {
+            error: true,
+            status: 404,
+            message: 'Space not found.',


Suggested change

message: 'Space not found.',

message: 'Space ${capability.with} has not storage provider. Storage provider can be added by invoking "voucher/claim" capability',

I would for something more descriptive with an actionable feedback

I agree it would be good to have actionable feedback in the error message. I defer to @hugomrdias on whether this is the best advice to give, as changing the message here would also require a change the tested behavior, and would prefer to have that change be in separate PR if needed.

gobengo · 2023-01-28T00:49:19Z

also relevant: storacha/specs#18

… instead of status integer

gobengo · 2023-01-30T18:58:10Z

I updated the change here based on review of @hugomrdias and @Gozala . Now the union tag to detect this failure type is name equaling SpaceUnknown instead of anything with a status property. This should still be enough for the use case at hand, which is upload-api having enough info on the result of space/info that it can distinguish in verifiyInvocation between an invalid invocation and a valid invocation to get info for an unknown space that access-api does not want to (or is unable to) serve info for.

hugomrdias

LGTM, left a comment inline but not blocking

hugomrdias · 2023-01-30T18:57:13Z

packages/access-client/src/errors.ts

+ * e.g. it's a space that's never been seen before,
+ * or it's a seen space that hasn't been fully registered such that the service can serve info about the space.
+ */
+export interface SpaceUnknown {


we probably should extends Failure here same as other errors in ucanto

I had omitted it on purpose so that the canonical service types didn't need to depend on ucanto (and we'd just rely on tsc checking the structural types), but don't have strong opinions on it either way, so I added the extends clause 0b63c2e .

…ilure

🤖 I have created a release *beep* *boop* --- ## [4.9.0](access-api-v4.8.0...access-api-v4.9.0) (2023-01-30) ### Features * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([9610fcf](9610fcf)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([50c0c80](50c0c80)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([71cbeb7](71cbeb7)) ### Bug Fixes * make tests use did:web everywhere ([#397](#397)) ([c7d5c34](c7d5c34)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [9.3.0](access-v9.2.0...access-v9.3.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([b773376](b773376)) * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([9610fcf](9610fcf)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([50c0c80](50c0c80)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([71cbeb7](71cbeb7)) ### Bug Fixes * remove unecessary awaits ([#352](#352)) ([64da6e5](64da6e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…ure with name (#391) Motivation: * implement #382 * tl;dr `space/info` now has explicit failure type for case where the ucans are valid, but the space DID is not in the db. that way when upload-api invokes `space/info` as part of its `verifyInvocation` logic, it can distinguish between an invalid invocation and a not-found space id Unblocks: * this test passing gobengo/w3protocol-test#1 * rest of storacha/w3infra#134 (comment)

🤖 I have created a release *beep* *boop* --- ## [4.9.0](access-api-v4.8.0...access-api-v4.9.0) (2023-01-30) ### Features * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([665dac9](665dac9)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([ec39443](ec39443)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([163fb74](163fb74)) ### Bug Fixes * make tests use did:web everywhere ([#397](#397)) ([00be288](00be288)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- ## [9.3.0](access-v9.2.0...access-v9.3.0) (2023-01-30) ### Features * access-api forwards store/ and upload/ invocations to upload-api ([#334](#334)) ([6be7217](6be7217)) * access-api handling store/info for space not in db returns failure with name ([#391](#391)) ([665dac9](665dac9)) * update @ucanto/* to ~4.2.3 ([#405](#405)) ([ec39443](ec39443)) * update access-api ucanto proxy to not need a signer ([#390](#390)) ([163fb74](163fb74)) ### Bug Fixes * remove unecessary awaits ([#352](#352)) ([2e8c1a1](2e8c1a1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

access-api handling store/info for space not in db returns failure wi…

44e8232

…th status=404

gobengo changed the title ~~access-api handling store/info for space not in db returns failure with status=404~~ feat: access-api handling store/info for space not in db returns failure with status=404 Jan 24, 2023

gobengo temporarily deployed to dev January 24, 2023 00:51 — with GitHub Actions Inactive

gobengo requested review from hugomrdias, alanshaw and Gozala January 24, 2023 00:56

hugomrdias suggested changes Jan 24, 2023

View reviewed changes

gobengo commented Jan 24, 2023

View reviewed changes

packages/access-client/src/types.ts Outdated Show resolved Hide resolved

Gozala requested changes Jan 24, 2023

View reviewed changes

gobengo changed the title ~~feat: access-api handling store/info for space not in db returns failure with status=404~~ feat: access-api handling store/info for space not in db returns failure with name=SpaceNotFound Jan 28, 2023

gobengo changed the title ~~feat: access-api handling store/info for space not in db returns failure with name=SpaceNotFound~~ feat: access-api handling store/info for space not in db returns failure with name Jan 30, 2023

change SpaceNotFoundFailure -> SpaceUnknown, which uses a name string…

dd82bbf

… instead of status integer

gobengo force-pushed the 1674521260-spaceinfo-notfound branch from 8f0e5e3 to dd82bbf Compare January 30, 2023 18:52

gobengo temporarily deployed to dev January 30, 2023 18:53 — with GitHub Actions Inactive

gobengo requested review from hugomrdias and Gozala January 30, 2023 18:55

gobengo marked this pull request as ready for review January 30, 2023 18:55

hugomrdias approved these changes Jan 30, 2023

View reviewed changes

access-client/src/errors.ts SpaceUnknown explicitly extends Ucanto.Fa…

0b63c2e

…ilure

gobengo merged commit 9610fcf into main Jan 30, 2023

gobengo deleted the 1674521260-spaceinfo-notfound branch January 30, 2023 21:21

gobengo temporarily deployed to dev January 30, 2023 21:26 — with GitHub Actions Inactive

This was referenced Jan 30, 2023

chore(main): release access 9.3.0 #346

Merged

chore(main): release access-api 4.9.0 #400

Merged

gobengo mentioned this pull request Jan 31, 2023

Add test for space/info delegation gobengo/w3protocol-test#1

Merged

it-dag-house mentioned this pull request Mar 23, 2023

chore(main): release access-api 11.0.0-rc.0 #620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: access-api handling store/info for space not in db returns failure with name #391

feat: access-api handling store/info for space not in db returns failure with name #391

gobengo commented Jan 24, 2023 •

edited

Loading

hugomrdias Jan 24, 2023

hugomrdias Jan 24, 2023

gobengo Jan 24, 2023 •

edited

Loading

hugomrdias Jan 25, 2023

gobengo Jan 28, 2023

gobengo Jan 30, 2023

hugomrdias Jan 24, 2023

gobengo Jan 30, 2023

Gozala left a comment

Gozala Jan 24, 2023

Gozala Jan 24, 2023

gobengo Jan 30, 2023

Gozala Jan 24, 2023

gobengo Jan 30, 2023 •

edited

Loading

gobengo commented Jan 28, 2023

gobengo commented Jan 30, 2023

hugomrdias left a comment

hugomrdias Jan 30, 2023

gobengo Jan 30, 2023 •

edited

Loading

	message: 'Space not found.',
	message: 'Space ${capability.with} has not storage provider. Storage provider can be added by invoking "voucher/claim" capability',

feat: access-api handling store/info for space not in db returns failure with name #391

feat: access-api handling store/info for space not in db returns failure with name #391

Conversation

gobengo commented Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Jan 24, 2023 • 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

Gozala 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

gobengo Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

gobengo commented Jan 28, 2023

gobengo commented Jan 30, 2023

hugomrdias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gobengo Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

gobengo commented Jan 24, 2023 •

edited

Loading

gobengo Jan 24, 2023 •

edited

Loading

gobengo Jan 30, 2023 •

edited

Loading

gobengo Jan 30, 2023 •

edited

Loading