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

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

Merged
merged 3 commits into from
Jan 30, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44e8232
access-api handling store/info for space not in db returns failure wi…
gobengo Jan 24, 2023
dd82bbf
change SpaceNotFoundFailure -> SpaceUnknown, which uses a name string…
gobengo Jan 30, 2023
0b63c2e
access-client/src/errors.ts SpaceUnknown explicitly extends Ucanto.Fa…
gobengo Jan 30, 2023
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
6 changes: 5 additions & 1 deletion packages/access-api/src/service/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ export function service(ctx) {
info: Server.provide(Space.info, async ({ capability, invocation }) => {
const results = await ctx.models.spaces.get(capability.with)
if (!results) {
return new Failure('Space not found.')
return {
error: true,
status: 404,
message: 'Space not found.',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@gobengo gobengo Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gobengo gobengo Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

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 updated [here] to not have a status at all and just a well-defined name 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

}
return results
}),
Expand Down
5 changes: 5 additions & 0 deletions packages/access-api/test/space-info.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ describe('space/info', function () {

if (inv?.error) {
assert.deepEqual(inv.message, `Space not found.`)
assert.deepEqual(
'status' in inv && inv.status,
404,
'error result has status 404'
)
} else {
assert.fail()
}
Expand Down
13 changes: 12 additions & 1 deletion packages/access-client/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ export interface SpaceTableMetadata {
agent: AgentMeta
}

/**
* Indicates failure executing ability that requires access to a space that cannot be found by the handler
*/
export type SpaceNotFoundFailure = Failure & {
status: 404
gobengo marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

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'

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 got rid of status and used name of UnknownSpace in 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 moved the failure type to an errors.ts file as you suggested 8f0e5e3#diff-f4e938a2d743868395a4bb96adeecd10ce306ac53100e1e978df8d40ac33d4ef

/**
* Access api service definition type
*/
Expand All @@ -73,7 +80,11 @@ export interface Service {
redeem: ServiceMethod<VoucherRedeem, void, Failure>
}
space: {
info: ServiceMethod<SpaceInfo, Selectable<SpaceTable>, Failure>
info: ServiceMethod<
SpaceInfo,
Selectable<SpaceTable>,
Failure | SpaceNotFoundFailure
>
'recover-validation': ServiceMethod<
SpaceRecoverValidation,
EncodedDelegation<[SpaceRecover]> | undefined,
Expand Down