-
Notifications
You must be signed in to change notification settings - Fork 11
feat: queryOffset to return RMQ response code in the domain exception (resolves #283) #285
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
73996df
4f887e8
835e4d9
9477d03
99cee43
4890110
4b642bb
2c44847
09ec85b
a8ea375
1c5a897
1731ad8
1b4b359
6ddcd9b
ed50fd6
8dadbb9
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,62 @@ | ||
| "use strict" | ||
|
|
||
| import { ResponseCode } from "../util" | ||
|
|
||
| export type TResponseCode = (typeof ResponseCode)[keyof typeof ResponseCode] | ||
|
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 think this type is necessary, we can just use a type number for the code
Author
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. @l4mby I see. export default class RMQProtocolResponseError extends Error {
readonly #code: TResponseCode
constructor(message: string, rmqStreamResponseCode: TResponseCode) {
super(message)The retionale to narrow type of The type alias
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. Hi @WhereJuly the problem with this type const code = res.code as TResponseCodeAnd if you receive a number that is not present in About the name, it could be useful to have an error we use instead of "generic error" but it not be too generic because not always can be used so I suggest: So this code could became: export class StreamResponseError extends Error {
constructor(message: string, readonly errorCode: number) {
super(message)
}
}If you want to use the |
||
|
|
||
| /** | ||
| * Provides distinct domain exception for the package. Contains the optional | ||
| * RabbitMQ Stream protocol response code for more convenient processing. | ||
| * | ||
| * @param message A custom error message. | ||
| * @param rmqStreamResponseCode The above mentioned response code. | ||
| * | ||
| * @see https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbitmq_stream/docs/PROTOCOL.adoc#response-codes | ||
| * | ||
| * @example Selectively manage the exception type and react differently. | ||
| * | ||
| * ```typescript | ||
| * let result: any; | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * | ||
| * const isRethrowable = (error_: Error) => { | ||
| * const isGenericError = error_ instanceof Code51Exception; | ||
| * const isNonManagedResponseCode = (error_ as Code51Exception).code !== ResponseCode.NoOffset; | ||
| * | ||
| * return isGenericError && isNonManagedResponseCode; | ||
| * }; | ||
| * | ||
| * try { | ||
| * result = consumer.queryOffset(); | ||
| * // ... process result | ||
| * } catch (error_) { | ||
| * if (isRethrowable(error_)) { throw error_; } | ||
| * | ||
| * const error = error_ as Code51Exception; | ||
| * if (error.code === ResponseCode.NoOffset) { return null; } | ||
| * | ||
| * return result; | ||
| * } | ||
| * ``` | ||
| * | ||
|
Comment on lines
+7
to
+40
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 suggest to keep the description related to the field of class. function isStreamResponseError(error: unknown): error is StreamResponseError {
return ... // shoudl return a boolean value
} |
||
| */ | ||
| export default class Code51Exception extends Error { | ||
| readonly #code?: TResponseCode | ||
|
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. We should use a more appropriate name for the error, specific to the queryoffset, for example
Author
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. @l4mby Thank you. Understood. Wanted to discuss the following points.
Will do.
I planned this error to be a generic error for the entire package. Now I see this seems too wide. On the other hand maybe Could it be better
I could but this is not name convention, The reason to have it is to actually make it read-only at run time, accessibe only via a getter. Making it |
||
|
|
||
| constructor(message: string, rmqStreamResponseCode?: TResponseCode) { | ||
| super(message) | ||
|
|
||
| Object.setPrototypeOf(this, new.target.prototype) | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| this.name = this.constructor.name | ||
| this.#code = rmqStreamResponseCode ?? undefined | ||
|
|
||
| // Maintains proper stack trace for where our error was thrown (only available on V8) | ||
| if (Error.captureStackTrace) { | ||
| Error.captureStackTrace(this, this.constructor) | ||
| } | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| public get code(): TResponseCode | undefined { | ||
| return this.#code | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,7 @@ import { coerce, lt } from "semver" | |
| import EventEmitter from "events" | ||
| import { MetadataUpdateResponse } from "./responses/metadata_update_response" | ||
| import { MetadataInfo } from "./responses/raw_response" | ||
| import Code51Exception, { TResponseCode } from "./application/Code51Exception" | ||
|
|
||
| export type ConnectionClosedListener = (hadError: boolean) => void | ||
|
|
||
|
|
@@ -561,15 +562,70 @@ export class Connection { | |
| return res.sequence | ||
| } | ||
|
|
||
| /** | ||
| * Store the provided offset at the RabbitMQ server in the given stream for the given consumer. | ||
| * | ||
| * @param StoreOffsetParams The stream name, consumer name and the offset value. | ||
| * | ||
| * The offset is stored on the given stream as the additional service message not visible to | ||
| * a stream consumers but counted for in the RabbitMQ UI. | ||
| * | ||
| * For streams with millions of messages per second it is recommended to store the offset | ||
| * once per a number of messages to not litter the stream and potentially worsen the performance | ||
| * when very high throughput is required. | ||
| * | ||
| * @see https://www.rabbitmq.com/blog/2021/09/13/rabbitmq-streams-offset-tracking#the-dark-side-of-server-side-offset-tracking | ||
| * @see https://www.rabbitmq.com/docs/streams#offset-tracking | ||
| */ | ||
| public storeOffset(params: StoreOffsetParams): Promise<void> { | ||
| return this.send(new StoreOffsetRequest(params)) | ||
| } | ||
|
|
||
| /** | ||
| * Return the server-side saved offset or throws {@link Code51Exception} with the | ||
| * RabbitMQ response code. | ||
| * | ||
| * @see https://www.rabbitmq.com/tutorials/tutorial-two-javascript-stream | ||
| * @see https://www.rabbitmq.com/blog/2021/09/13/rabbitmq-streams-offset-tracking | ||
| * | ||
| * @param QueryOffsetParams The stream name and the named consumer identifier object. | ||
| * | ||
| * @see {@link Connection.connect} | ||
| * @see {@link Connection.storeOffset} | ||
| * | ||
| * @example Consumer reads previously saved server-tracked offset to start consuming | ||
| * from the desired offset. | ||
| * | ||
| * On consumer side create the client, detect the server-side saved offset to detect | ||
| * the desired offset to consume from. Then declare a consumer with the desired offset | ||
| * and the consumed message handler. | ||
| * | ||
| * When consuming, the consumer message handler may save the offset server-side | ||
| * for the `client.queryOffset()` to be able to further read it. | ||
| * | ||
| * ```typescript | ||
| * // ... create the RabbitMQ client here | ||
| * // Detect the decider starting offset for the next stream operation. | ||
| * const offset = await client.queryOffset({ reference: 'consumer-x', stream: 'stream-a' }) | ||
| * const startWithOffset = offset ? rmqLibrary.Offset.offset(offset + 1n) : | ||
| * rmqLibrary.Offset.<whatever-enum-offset-is-desired>(); | ||
| * const consumer = await client.declareConsumer({stream: 'stream-b', offset: startWithOffset}, | ||
| * async (this: StreamConsumer, message: Message) => { await this.storeOffset(message.offset); }); | ||
| * // Note the offset is saved by the message handler on the server. | ||
| * ``` | ||
| * | ||
| * @throws {@link Code51Exception} if the server-side offset cannot be retrieved. The exception | ||
| * contains the `code` field that equals the RabbitMQ stream protocol response code value. | ||
| * | ||
| * @see https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbitmq_stream/docs/PROTOCOL.adoc#response-codes | ||
| */ | ||
| public async queryOffset(params: QueryOffsetParams): Promise<bigint> { | ||
| this.logger.debug(`Query Offset...`) | ||
| const res = await this.sendAndWait<QueryOffsetResponse>(new QueryOffsetRequest(params)) | ||
| if (!res.ok) { | ||
| throw new Error(`Query offset command returned error with code ${res.code}`) | ||
| const code = res.code as TResponseCode | ||
|
|
||
| throw new Code51Exception(`Query offset command returned error with code ${res.code}`, 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. Here since we will use the code as a number we can remove the as and throw directly the code in the constructor of
Author
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. @l4mby For this case I made an explanation here #285 (comment). Shortly, my logic here is we better use the narrower type
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. Hi @WhereJuly as written above we try to avoid |
||
| } | ||
| this.logger.debug(`Query Offset response: ${res.ok} with params: '${inspect(params)}'`) | ||
| return res.offsetValue | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,9 +48,23 @@ export const wait = async (ms: number) => { | |
| }) | ||
| } | ||
|
|
||
| /** | ||
| * The RabbitMQ Stream protocol response codes. | ||
| * | ||
| * Only codes actually used by this package are defined here however any official code | ||
| * can be returned by the actually connected RabbitMQ instance. | ||
| * | ||
| * @see https://github.com/rabbitmq/rabbitmq-server/blob/v3.9.x/deps/rabbitmq_stream/docs/PROTOCOL.adoc#response-codes | ||
| * | ||
| * @see {@link connection.ts/Connection.queryOffset} | ||
| * @see {@link application/Code51Exception} | ||
| */ | ||
| export const ResponseCode = { | ||
| StreamDoesNotExist: 2, | ||
| SubscriptionIdDoesNotExist: 4, | ||
|
|
||
| // Used in src/connection.ts/Connection.queryOffset method | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
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 comment could became stale easely please remove it. |
||
| NoOffset: 19, | ||
| } as const | ||
|
|
||
| export const isString = (value: unknown): boolean => typeof value === "string" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { expect } from "chai" | ||
|
|
||
| import Code51Exception from "../../../src/application/Code51Exception" | ||
| import { ResponseCode } from "../../../src/util" | ||
|
|
||
| describe("[unit] Code51Exception Test", () => { | ||
| it("+constructor() #1: Should create Code51Exception expected default object", () => { | ||
| const expected = "A message" | ||
| const actual = new Code51Exception(expected) | ||
|
|
||
| expect(actual).instanceOf(Code51Exception) | ||
| expect(actual.message).eql(expected) | ||
| expect(actual.code).eql(undefined) | ||
| }) | ||
|
|
||
| it("+constructor() #2: Should create Code51Exception expected object with expected response code", () => { | ||
| const expected = ResponseCode.NoOffset | ||
| const actual = new Code51Exception("a message", expected) | ||
|
|
||
| expect(actual.code).eql(expected) | ||
| }) | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| it("Should throw expected Code51Exception exception", () => { | ||
| const expected = { message: "A message", code: ResponseCode.SubscriptionIdDoesNotExist } | ||
|
|
||
| try { | ||
| throw new Code51Exception(expected.message, expected.code) | ||
| } catch (error_) { | ||
| const actual = error_ as Code51Exception | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| expect(actual).instanceOf(Code51Exception) | ||
| expect(actual.message).eql(expected.message) | ||
| expect(actual.code).eql(expected.code) | ||
| } | ||
| }) | ||
| }) | ||
|
|
||
| // Assert: proper stack trace | ||
WhereJuly marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.