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 2 commits
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
8 changes: 7 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,13 @@ 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.')
/** @type {import('@web3-storage/access/types').SpaceUnknown} */
const spaceUnknownFailure = {
error: true,
name: 'SpaceUnknown',
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.

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 spaceUnknownFailure
}
return results
}),
Expand Down
6 changes: 6 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,12 @@ describe('space/info', function () {

if (inv?.error) {
assert.deepEqual(inv.message, `Space not found.`)
const expectedErrorName = 'SpaceUnknown'
assert.deepEqual(
'name' in inv && inv.name,
expectedErrorName,
`error result has name ${expectedErrorName}`
)
} else {
assert.fail()
}
Expand Down
10 changes: 10 additions & 0 deletions packages/access-client/src/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Indicates failure executing ability that requires access to a space that is not well-known enough to be handled.
* 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 {
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 extends Failure here same as other errors in ucanto

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 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 .

error: true
message: string
name: 'SpaceUnknown'
}
8 changes: 7 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 @@ -35,9 +35,11 @@ import type {
import type { SetRequired } from 'type-fest'
import { Driver } from './drivers/types.js'
import type { ColumnType, Selectable } from 'kysely'
import { SpaceUnknown } from './errors.js'

// export other types
export * from '@web3-storage/capabilities/types'
export * from './errors.js'

/**
* D1 Types
Expand Down Expand Up @@ -73,7 +75,11 @@ export interface Service {
redeem: ServiceMethod<VoucherRedeem, void, Failure>
}
space: {
info: ServiceMethod<SpaceInfo, Selectable<SpaceTable>, Failure>
info: ServiceMethod<
SpaceInfo,
Selectable<SpaceTable>,
Failure | SpaceUnknown
>
'recover-validation': ServiceMethod<
SpaceRecoverValidation,
EncodedDelegation<[SpaceRecover]> | undefined,
Expand Down