[Question] Query offset command returned error with code 19

[WhereJuly]

Hello, I try to understand if that exception should be actually treated as the case "query offset is not set" or not.

That comes from the line

const offset = await client.queryOffset({ reference: consumerRef, stream: streamName })

when I try to get the stream offset.

Do I suggest correctly that the code 19 is NO_OFFSET (19) - no stored offset (see)?

Woudl it be better to explicitly re-throw the original error or the wrapped one with the original error code as field, not just a generic Error with Query offset command returned error with code 19 message? This prevents from decide what exactly was the problem.

Or, if the code is 19 just means "offset is not defined" - it is not an error code, and the method should return undefined instead of throwing for code 19.

Kind of:

  public async queryOffset(params: QueryOffsetParams): Promise<bigint> {
    this.logger.debug(`Query Offset...`)
    const res = await this.sendAndWait<QueryOffsetResponse>(new QueryOffsetRequest(params))
    
    // The NO_OFFSET is returned, meaning no offset was stored for this consumer. Not an error in fact.
    // see https://github.com/rabbitmq/rabbitmq-server/blob/v3.9.x/deps/rabbitmq_stream/docs/PROTOCOL.adoc#response-codes
    if(!res.ok && res.code === 19) { return undefined; }

    // For any other codes, we re-throws the original response to make the codes available to the caller
    if (!res.ok) { throw res;  }

    this.logger.debug(`Query Offset response: ${res.ok} with params: '${inspect(params)}'`)

    return res.offsetValue
  }

Would the PR be of a value?

[l4mby]

Hello @WhereJuly, thank you for opening the issue. At the moment we cannot return undefined because it will change the response of the api. What we could do is throw a new Error with inside the code field, if you want to contribute with a PR with this method we'll evaluate it asap.

[WhereJuly]

@l4mby Thank you. I understand. I will make a PR in the coming days.

Discussion

For a discussion, I believe we could make it open/closed way for the sake of developer (the package user) experience. In order not to make a developer to wrap the call in try/catch we could add a parameter to optionally change the .queryOffset() behavior. Here is the signature, potentially:

/**
 * ... other jsdoc here
 *  @params throwForNoOffset Optionally makes the method not to throw for the case when the RMQ response code
 *  is NO_OFFSET (19) (see ... link to the protocol response codes table)
 */
public async queryOffset(params: QueryOffsetParams, throwForNoOffset: boolean = true): Promise<bigint> | null;

By default, throwForNoOffset is true and the method behavior does not change, being backward-compatible, throwing an exceptiion, probably not a generic Error, but the domain one, like C51RabbitMQStreamClient containg the code filed, anticipating the user potential checks by the error type.

When throwForNoOffset is false the method returns null only for the NO_OFFSET (19) RMQ stream protocol response, still raising exceptions for all other cases.

NB: The initial undefined suggestion was inconsistent, I usually use null for the things a programmer impacts, leaving undefined for where it was actually not defined by the programmer.

Usage

The usage example would be more concise and flat then (without the necessity to wrap the entire success case in try/catch). E.g.

const offset = await client.queryOffset({ reference: consumerRef, stream: streamName}, false); 
const startWithOffset = offset ?
    rmqLibrary.Offset.offset(offset + <whatever-bigint-is-required>) : 
    rmqLibrary.Offset.<whatever-enum-offset-is-desired>();

const consumerOptions: DeclareConsumerParams = { stream: streamName, consumerRef, offset: startWithOffset };
const consumer = await helper.client.declareConsumer(consumerOptions, messageHandler);

So it does not change the existing behavior but adds to it optionally.

What do you think?

[WhereJuly]

@l4mby Hello, just submitted the PR #285 for your review. Thank you.