feat: queryOffset to return RMQ response code in the domain exception (resolves #283) #285

WhereJuly · 2025-11-12T07:20:35Z

Hey Team,

In response to #283 issue.

Before: the src\connection.ts->Connection.queryOffset threw the generic Error with a string variable for any RabbitMQ stream protocol response code including NO_OFFSET (19). See more in [Question] Query offset command returned error with code 19 #283.
After: the src\connection.ts->Connection.queryOffset throws the domain exception src\application\Code51Exception.ts with a dedicated code field. See the usage example in the class JSDoc block.

The main changes introduced:

The domain exception src\application\Code51Exception.ts to carry the response code along the error message, tested with test\unit\application\Code51Exception.test.ts
A NoOffset member intosrc\util.ts->ResponseCode
A src\application\Code51Exception.ts->TResponseCode type alias to simplify the response code parameters description.

A number of JSDoc blocks in:

src\connection.ts: storeOffset, queryOffset
src\util.ts: ResponseCode
src\client.ts: DeclareConsumerParams, StoreOffsetParams, QueryOffsetParams

The CI attempt went well.

Thanks.

l4mby · 2025-11-12T15:00:54Z

Hello @WhereJuly, please keep the PR focused on a single topic.
So I suggest to keep this PR only related to the NO_OFFSET code issue and open a separate PR for the JSDoc.
Also the code related to the NO_OFFSET should be improved, i will write down some comments asap.

WhereJuly · 2025-11-12T15:46:01Z

Hey @l4mby. Thanks for the comment. The JSDoc blocks naturally followed the new functionality implementation. They are, kind of, immanent part of the PR, induced by the changes. Probably, formal separation is not appropriate here. What do you think?

l4mby

We reviewed your code and found some inconsistencies with our style. Please fix it so we can proceed with the integration. Thank you.
PS: Did you use any AI tool? If yes can you please tell us which tool/model you have used?

src/util.ts

l4mby · 2025-11-14T08:08:37Z

src/application/Code51Exception.ts

+
+import { ResponseCode } from "../util"
+
+export type TResponseCode = (typeof ResponseCode)[keyof typeof ResponseCode]


I don't think this type is necessary, we can just use a type number for the code

@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 rmqStreamResponseCode is to not allow to provide any number whereas we actually expect only ResponseCode.

The type alias TResponseCode is for better clarity and readability - semantically and to not use that long typecast. So I believe we should keep it. Let me know your thought on this.

Hi @WhereJuly the problem with this type TResponseCode is that force you to use the as like in the code below

const code = res.code as TResponseCode

And if you receive a number that is not present in ResponseCode you are forcing it. This is the reason about using a number and left the user to verify if it's a known code present in ResponseCode.

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: class StreamResponseError.

So this code could became:

export class StreamResponseError extends Error { constructor(message: string, readonly errorCode: number) { super(message) } }

If you want to use the #errorCode instead the typescript version, it's fine I suggest to name the property errorCode or responseCode.
As side note we prefer don't use default export.

src/application/Code51Exception.ts

l4mby · 2025-11-14T08:08:44Z

src/application/Code51Exception.ts

+ *
+ */
+export default class Code51Exception extends Error {
+  readonly #code?: TResponseCode


We should use a more appropriate name for the error, specific to the queryoffset, for example QueryOffsetError could be ok. Also can you rename the property in code and make it mandatory?

@l4mby Thank you. Understood. Wanted to discuss the following points.

code: make it mandatory

Will do.

more appropriate name for the error, specific to the queryoffset, for example QueryOffsetError

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 QueryOffsetError is too specific?

Could it be better RMQProtocolResponseError or something like that to cater for all potential use cases with the protocol response code? To not end up with a number of several too specific error classes?

Also can you rename the property in code

I could but this is not name convention, # is the JS property visiblity level. It actually makes the JS (not TS) property private at runtime.

The reason to have it is to actually make it read-only at run time, accessibe only via a getter. Making it code we expose it at runtime. Do you still think we should not use JS private properties?

l4mby · 2025-11-14T08:08:58Z

src/connection.ts

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


Here since we will use the code as a number we can remove the as and throw directly the code in the constructor of QueryOffsetError

@l4mby For this case I made an explanation here #285 (comment).

Shortly, my logic here is we better use the narrower type ResponseCode than wider number as the error class is going to accept only ResponseCode, so no reason to relax the expected type.

Hi @WhereJuly as written above we try to avoid as because it's very dangerous.

test/e2e/offset.test.ts

test/unit/application/Code51Exception.test.ts

WhereJuly · 2025-11-14T10:58:57Z

Hello @l4mby. Thanks for the comments. Will return with the updated PR in the coming days.

As I mentioned, are you OK with keeping the JSDocs related to exactly this PR in it?

PS: Did you use any AI tool? If yes can you please tell us which tool/model you have used?

For code I rarely use AI meaning there is no lines of code produced by AI and copy/pasted in my code.

I do use Cline for VS Code code base explorations, sometimes debugging (with free Gemini flash 2.5 I believe). Outside VS Code I use ChatGPT Pro (earlier v3.5, v4o, v5 now) for many things code-wise except writing code. And if used as input, I always edit it heavily before putting into a codebase.

Hope that answers the question :)

Talk to you soon.

l4mby · 2025-11-14T11:02:46Z

Hi @WhereJuly thank you for the infos. Yes we talked about it internally and decided it's ok to keep the JSDoc in the PR.
Thank you again in advance

WhereJuly · 2025-11-14T11:05:48Z

@l4mby Welcome and thank you :) I meant to make a separate PRs for the JSDocs when there is no accompanied code comes with that. Will do that in the future.

- tdd the exception; - update src/utils.ts ResponseCode to add NoOffset member and add jsdoc;

…se code

WhereJuly · 2025-11-18T05:12:03Z

@l4mby Messed with git history. Will make updates and open the new PR.

WhereJuly · 2025-11-18T06:35:44Z

Hey @l4mby. I made code updates corresponding to the resolved comments to keep them closed and added my rarionale and questions for 3 still open conversations. Can you, please, let me know your view on the pending points? Thank you :)

And besdes the main subject, I am interested why you asked about my usage of AI? And what is your own stance on using it? Thank you.

WhereJuly · 2025-11-21T06:15:40Z

Hey @l4mby, just checking in, wanted to make sure my comments still reached you after the PR was closed. Reopened it just for the case.

gpad · 2025-11-27T07:49:24Z

src/application/Code51Exception.ts

+
+import { ResponseCode } from "../util"
+
+export type TResponseCode = (typeof ResponseCode)[keyof typeof ResponseCode]


Hi @WhereJuly the problem with this type TResponseCode is that force you to use the as like in the code below

const code = res.code as TResponseCode

And if you receive a number that is not present in ResponseCode you are forcing it. This is the reason about using a number and left the user to verify if it's a known code present in ResponseCode.

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: class StreamResponseError.

So this code could became:

export class StreamResponseError extends Error { constructor(message: string, readonly errorCode: number) { super(message) } }

If you want to use the #errorCode instead the typescript version, it's fine I suggest to name the property errorCode or responseCode.
As side note we prefer don't use default export.

gpad · 2025-11-27T07:51:27Z

src/connection.ts

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


Hi @WhereJuly as written above we try to avoid as because it's very dangerous.

gpad · 2025-11-27T07:52:07Z

src/util.ts

  StreamDoesNotExist: 2,
  SubscriptionIdDoesNotExist: 4,
+
+  // Used in src/connection.ts/Connection.queryOffset method


This comment could became stale easely please remove it.

gpad · 2025-11-27T07:57:18Z

src/application/Code51Exception.ts

+/**
+ * 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;
+ *
+ * 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;
+ * }
+ * ```
+ *


I suggest to keep the description related to the field of class.
We could provide a guard function in this file to verify if the error is StreamResponseError something like:

function isStreamResponseError(error: unknown): error is StreamResponseError { return ... // shoudl return a boolean value }

WhereJuly · 2025-11-27T09:12:47Z

Hey @gpad, thanks for the response :) I just had a quick glance, it all looked fine. I will return to the PR in the coming days. Hope to complete it then. Cheers.

WhereJuly mentioned this pull request Nov 12, 2025

[Question] Query offset command returned error with code 19 #283

Closed

l4mby reviewed Nov 14, 2025

View reviewed changes

WhereJuly closed this Nov 18, 2025

WhereJuly deleted the 283-queryoffset-code-in-exception branch November 18, 2025 04:44

WhereJuly restored the 283-queryoffset-code-in-exception branch November 18, 2025 04:52

WhereJuly reopened this Nov 18, 2025

WhereJuly added 16 commits November 18, 2025 08:02

Add jsdoc to the original queryOffset method

73996df

Implement Code51Exception

4f887e8

- tdd the exception; - update src/utils.ts ResponseCode to add NoOffset member and add jsdoc;

Update Code51Exception: minor

835e4d9

Update QueryOffsetParams: add jsdoc

9477d03

Update StoreOffsetParams: add jsdoc

99cee43

Update Connection.storeOffset: add jsdoc

4890110

Update Code51Exception: export a type

4b642bb

Update offset test: a typo

2c44847

Add jsdoc to DeclareConsumerParams interface

09ec85b

Update Connection.queryOffset: implement custom exception with respon…

a8ea375

…se code

Add jsdoc to Code51Exception

1c5a897

Update queryOffset: jsdoc

1731ad8

Update GitHub CI: checking tests success

1b4b359

Update misc files: typos

6ddcd9b

Restore: skipped brittle test

ed50fd6

Clean up: CI test branch trigger

8dadbb9

WhereJuly force-pushed the 283-queryoffset-code-in-exception branch from d50d242 to 8dadbb9 Compare November 18, 2025 05:04

WhereJuly closed this Nov 18, 2025

WhereJuly reopened this Nov 21, 2025

WhereJuly requested a review from l4mby November 21, 2025 06:07

gpad requested changes Nov 27, 2025

View reviewed changes


		import { ResponseCode } from "../util"

		export type TResponseCode = (typeof ResponseCode)[keyof typeof ResponseCode]

feat: queryOffset to return RMQ response code in the domain exception (resolves #283) #285

Are you sure you want to change the base?

feat: queryOffset to return RMQ response code in the domain exception (resolves #283) #285

Uh oh!

Conversation

WhereJuly commented Nov 12, 2025

Uh oh!

l4mby commented Nov 12, 2025

Uh oh!

WhereJuly commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l4mby left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WhereJuly Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WhereJuly commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l4mby commented Nov 14, 2025

Uh oh!

WhereJuly commented Nov 14, 2025

Uh oh!

WhereJuly commented Nov 18, 2025

Uh oh!

WhereJuly commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WhereJuly commented Nov 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WhereJuly commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WhereJuly commented Nov 12, 2025 •

edited

Loading

WhereJuly Nov 18, 2025 •

edited

Loading

WhereJuly commented Nov 14, 2025 •

edited

Loading

WhereJuly commented Nov 18, 2025 •

edited

Loading