-
Notifications
You must be signed in to change notification settings - Fork 9
Error handling of getStream/createStream/etc. #208
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| export enum ErrorCode { | ||
| NOT_FOUND = 'NOT_FOUND', | ||
| VALIDATION_ERROR = 'VALIDATION_ERROR', | ||
| UNKNOWN = 'UNKNOWN' | ||
| } | ||
|
|
||
| export const parseErrorCode = (body: string) => { | ||
| let json | ||
| try { | ||
| json = JSON.parse(body) | ||
| } catch (err) { | ||
| return ErrorCode.UNKNOWN | ||
| } | ||
| const code = json.code | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah follow the eslint warning here, use object destructuring |
||
| const keys = Object.keys(ErrorCode) | ||
| if (keys.includes(code)) { | ||
| return code as ErrorCode | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using return code in ErrorCode ? code : ErrorCode.UNKNOWN |
||
| return ErrorCode.UNKNOWN | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,10 @@ import Stream, { StreamOperation, StreamProperties } from '../stream' | |
| import StreamPart from '../stream/StreamPart' | ||
| import { isKeyExchangeStream } from '../stream/KeyExchange' | ||
|
|
||
| import authFetch from './authFetch' | ||
| import authFetch, { AuthFetchError } from './authFetch' | ||
| import { Todo } from '../types' | ||
| import StreamrClient from '../StreamrClient' | ||
| import { ErrorCode } from './ErrorCode' | ||
| // TODO change this import when streamr-client-protocol exports StreamMessage type or the enums types directly | ||
| import { ContentType, EncryptionType, SignatureType, StreamMessageType } from 'streamr-client-protocol/dist/src/protocol/message_layer/StreamMessage' | ||
|
|
||
|
|
@@ -97,18 +98,11 @@ export class StreamEndpoints { | |
| } | ||
|
|
||
| const url = getEndpointUrl(this.client.options.restUrl, 'streams', streamId) | ||
| try { | ||
| const json = await authFetch<StreamProperties>(url, this.client.session) | ||
| return new Stream(this.client, json) | ||
| } catch (e) { | ||
| if (e.response && e.response.status === 404) { | ||
| return undefined | ||
| } | ||
| throw e | ||
| } | ||
| const json = await authFetch<StreamProperties>(url, this.client.session) | ||
| return new Stream(this.client, json) | ||
| } | ||
|
|
||
| async listStreams(query: StreamListQuery = {}) { | ||
| async listStreams(query: StreamListQuery = {}): Promise<Stream[]> { | ||
| this.client.debug('listStreams %o', { | ||
| query, | ||
| }) | ||
|
|
@@ -126,10 +120,10 @@ export class StreamEndpoints { | |
| // @ts-expect-error | ||
| public: false, | ||
| }) | ||
| return json[0] ? new Stream(this.client, json[0]) : undefined | ||
| return json[0] ? new Stream(this.client, json[0]) : Promise.reject(new AuthFetchError('', undefined, undefined, ErrorCode.NOT_FOUND)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh I see why you left Maybe could wrap that messiness up and expose an API like: AuthFetchError.NotFound = class NotFoundError extends AuthFetchError {
constructor(msg = '') {
super(AuthFetchError(msg, undefined, undefined, ErrorCode.NOT_FOUND))
}
}
new AuthFetchError.NotFound(`Could not find stream by name: ${name}`)Then later you could do uh, though I guess this would be misleading since other non- export function createAuthFetchError(msg, response, body) {
switch (parseErrorCode(body)) {
case ErrorCode.NOT_FOUND: return new AuthFetchError.NotFound(msg, response, body)
case ErrorCode. VALIDATION_ERROR: return new AuthFetchError.ValidationError(msg, response, body)
default: return new AuthFetchError(msg, response, body)
}
}Just a suggestion though, might not be worth it, your call. |
||
| } | ||
|
|
||
| async createStream(props: StreamProperties) { | ||
| async createStream(props?: StreamProperties) { | ||
| this.client.debug('createStream %o', { | ||
| props, | ||
| }) | ||
|
|
@@ -142,34 +136,31 @@ export class StreamEndpoints { | |
| body: JSON.stringify(props), | ||
| }, | ||
| ) | ||
| return json ? new Stream(this.client, json) : undefined | ||
| return new Stream(this.client, json) | ||
| } | ||
|
|
||
| async getOrCreateStream(props: { id?: string, name?: string }) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be: props: { id: string } | { name: string }then if ('id' in props) {
// etc
} |
||
| this.client.debug('getOrCreateStream %o', { | ||
| props, | ||
| }) | ||
| let json: any | ||
|
|
||
| // Try looking up the stream by id or name, whichever is defined | ||
| if (props.id) { | ||
| json = await this.getStream(props.id) | ||
| } else if (props.name) { | ||
| json = await this.getStreamByName(props.name) | ||
| } | ||
|
|
||
| // If not found, try creating the stream | ||
| if (!json) { | ||
| json = await this.createStream(props) | ||
| debug('Created stream: %s (%s)', props.name, json.id) | ||
| try { | ||
| if (props.id) { | ||
| const stream = await this.getStream(props.id) | ||
| return stream | ||
| } | ||
| const stream = await this.getStreamByName(props.name!) | ||
| return stream | ||
| } catch (err) { | ||
| const isNotFoundError = (err instanceof AuthFetchError) && (err.errorCode === ErrorCode.NOT_FOUND) | ||
| if (!isNotFoundError) { | ||
| throw err | ||
| } | ||
| } | ||
|
|
||
| // If still nothing, throw | ||
| if (!json) { | ||
| throw new Error(`Unable to find or create stream: ${props.name || props.id}`) | ||
| } else { | ||
| return new Stream(this.client, json) | ||
| } | ||
| const stream = await this.createStream(props) | ||
| debug('Created stream: %s (%s)', props.name, stream.id) | ||
| return stream | ||
| } | ||
|
|
||
| async getStreamPublishers(streamId: string) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,22 +2,25 @@ import fetch, { Response } from 'node-fetch' | |
| import Debug from 'debug' | ||
|
|
||
| import { getVersionString } from '../utils' | ||
| import { ErrorCode, parseErrorCode } from './ErrorCode' | ||
| import Session from '../Session' | ||
|
|
||
| export const DEFAULT_HEADERS = { | ||
| 'Streamr-Client': `streamr-client-javascript/${getVersionString()}`, | ||
| } | ||
|
|
||
| export class AuthFetchError extends Error { | ||
| response: Response | ||
| response?: Response | ||
| body?: any | ||
| errorCode?: ErrorCode | ||
|
|
||
| constructor(message: string, response: Response, body?: any) { | ||
| constructor(message: string, response?: Response, body?: any, errorCode?: ErrorCode) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe instead of passing |
||
| // add leading space if there is a body set | ||
| const bodyMessage = body ? ` ${(typeof body === 'string' ? body : JSON.stringify(body).slice(0, 1024))}...` : '' | ||
| super(message + bodyMessage) | ||
| this.response = response | ||
| this.body = body | ||
| this.errorCode = errorCode | ||
|
|
||
| if (Error.captureStackTrace) { | ||
| Error.captureStackTrace(this, this.constructor) | ||
|
|
@@ -75,6 +78,6 @@ export default async function authFetch<T extends object>(url: string, session?: | |
| return authFetch<T>(url, session, options, true) | ||
| } else { | ||
| debug('%d %s – failed', id, url) | ||
| throw new AuthFetchError(`Request ${id} to ${url} returned with error code ${response.status}.`, response, body) | ||
| throw new AuthFetchError(`Request ${id} to ${url} returned with error code ${response.status}.`, response, body, parseErrorCode(body)) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,10 @@ function TestStreamEndpoints(getName) { | |
| expect(stream.requireSignedData).toBe(true) | ||
| expect(stream.requireEncryptedData).toBe(true) | ||
| }) | ||
|
|
||
| it('invalid id', () => { | ||
| return expect(() => client.createStream({ id: 'invalid.eth/foobar' })).rejects.toThrow() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't always add them myself, but should probably put something in |
||
| }) | ||
| }) | ||
|
|
||
| describe('getStream', () => { | ||
|
|
@@ -62,26 +66,45 @@ function TestStreamEndpoints(getName) { | |
|
|
||
| it('get a non-existing Stream', async () => { | ||
| const id = `${wallet.address}/StreamEndpoints-integration-nonexisting-${Date.now()}` | ||
| const stream = await client.getStream(id) | ||
| expect(stream).toBe(undefined) | ||
| return expect(() => client.getStream(id)).rejects.toThrow() | ||
| }) | ||
| }) | ||
|
|
||
| describe('getStreamByName', () => { | ||
| it('get an existing Stream', async () => { | ||
| const stream = await client.createStream() | ||
| const existingStream = await client.getStreamByName(stream.name) | ||
| expect(existingStream.id).toEqual(stream.id) | ||
| }) | ||
|
|
||
| it('get a non-existing Stream', async () => { | ||
| const name = `${wallet.address}/StreamEndpoints-integration-nonexisting-${Date.now()}` | ||
| return expect(() => client.getStreamByName(name)).rejects.toThrow() | ||
| }) | ||
| }) | ||
|
|
||
| describe('getOrCreate', () => { | ||
| it('getOrCreate an existing Stream', async () => { | ||
| it('getOrCreate an existing Stream by name', async () => { | ||
| const existingStream = await client.getOrCreateStream({ | ||
| name: createdStream.name, | ||
| }) | ||
| expect(existingStream.id).toBe(createdStream.id) | ||
| expect(existingStream.name).toBe(createdStream.name) | ||
| }) | ||
|
|
||
| it('getOrCreate an existing Stream by id', async () => { | ||
| const existingStream = await client.getOrCreateStream({ | ||
| id: createdStream.id, | ||
| }) | ||
| expect(existingStream.id).toBe(createdStream.id) | ||
| expect(existingStream.name).toBe(createdStream.name) | ||
| }) | ||
|
|
||
| it('getOrCreate a new Stream by name', async () => { | ||
| const newName = uid('stream') | ||
| const newStream = await client.getOrCreateStream({ | ||
| name: newName, | ||
| }) | ||
|
|
||
| expect(newStream.name).toEqual(newName) | ||
| }) | ||
|
|
||
|
|
@@ -90,7 +113,6 @@ function TestStreamEndpoints(getName) { | |
| const newStream = await client.getOrCreateStream({ | ||
| id: newId, | ||
| }) | ||
|
|
||
| expect(newStream.id).toEqual(newId) | ||
| }) | ||
| }) | ||
|
|
@@ -201,7 +223,7 @@ function TestStreamEndpoints(getName) { | |
| describe('Stream deletion', () => { | ||
| it('Stream.delete', async () => { | ||
| await createdStream.delete() | ||
| expect(await client.getStream(createdStream.id)).toBe(undefined) | ||
| return expect(() => client.getStream(createdStream.id)).rejects.toThrow() | ||
| }) | ||
| }) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this file's contents should probably live in
authFetch.ts.