[ADDED] natsSubscription_GetID() and natsSubscription_GetSubject() #565
Conversation
src/nats.h
Outdated
| * @param subject if not `NULL`, the memory location where to store the subject name. | ||
| */ | ||
| NATS_EXTERN natsStatus | ||
| natsSubscription_GetInfo(natsSubscription* sub, int64_t* sid, char* subject); |
There was a problem hiding this comment.
Thank you for adding the doc! Since it looks like you are returning the subject from the subscription object, and not a copy, I usually make that a const char* and add a \warning in the doc to indicate that the returned string belongs to the subscription object and the user must not free this memory.
Here is an example for natsMsg_GetSubject: https://nats-io.github.io/nats.c/group__msg_group.html#gad7cad38946648e3047aa0665a3dd230a
There was a problem hiding this comment.
Valid remark. Changed to const and added the warning in the docs.
src/sub.c
Outdated
| @@ -1124,6 +1124,35 @@ natsSubscription_QueuedMsgs(natsSubscription *sub, uint64_t *queuedMsgs) | |||
| return s; | |||
| } | |||
|
|
|||
| natsStatus | |||
| natsSubscription_GetInfo(natsSubscription* sub, int64_t* sid, char* subject) | |||
There was a problem hiding this comment.
Although I like reducing the number of APIs, what if some day we think that we should return some more information from the subscription? If we think that may happen, then we should here have a GetSubject() and GetID() right out of the get go.
There was a problem hiding this comment.
I was also debating whether to split this into 2 separate API's or not. And it is indeed a bit random that these 2 unrelated fields are returned together.
I split them as you suggested.
I did have a look at the test file but as you might be able to tell i'm quite unfamiliar with C. I don't have a clue how to actually run/debug the tests so I hope you don't mind I didn't provide you with a test. |
src/nats.h
Outdated
| * Returns the id of the given subscription. | ||
| * | ||
| * @param sub the pointer to the #natsSubscription object. | ||
| * @param id if not `NULL`, the memory location where to store the subscription id. |
There was a problem hiding this comment.
But now that we make individual getters, it is a bit silly to support NULL for the return location. That is, why would one call this function without wanting to retrieve the id?
src/nats.h
Outdated
| * \warning The string belongs to the subscription and must not be freed. Copy it if needed. | ||
| * | ||
| * @param sub the pointer to the #natsSubscription object. | ||
| * @param subject if not `NULL`, the memory location where to store the subject name. |
There was a problem hiding this comment.
Same comment that for the ID. Now, as a convention, we could say that those getter return "invalid" values that the user would have to interpret as the subscription being closed/invalid. For instance, natsSubscription_GetID() could be returning a int64_t (instead of status), and assume that 0 means invalid/closed. And natsSubscription_GetSubject() could return NULL to indicate invalid/closed subscription (since empty subject is not valid).
src/nats.h
Outdated
| * @param subject if not `NULL`, the memory location where to store the subject name. | ||
| */ | ||
| NATS_EXTERN natsStatus | ||
| natsSubscription_GetSubject(natsSubscription* sub, const char* subject); |
There was a problem hiding this comment.
If we keep return status and return value through "out" parameter, then you need to have const char **subject here.
src/sub.c
Outdated
| } | ||
|
|
||
| natsStatus | ||
| natsSubscription_GetSubject(natsSubscription* sub, const char* subject) |
src/sub.c
Outdated
| SUB_DLV_WORKER_LOCK(sub); | ||
|
|
||
| if (subject != NULL) | ||
| subject = (const char*)sub->subject; |
There was a problem hiding this comment.
Would be *subject = (const char*) sub->subject;
…combined with an out parameter
src/sub.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| int64_t id = 0; |
There was a problem hiding this comment.
I prefer that we declare this at the top of the function (to be consistent with rest of the code).
src/sub.c
Outdated
| } | ||
|
|
||
| int64_t id = 0; | ||
| SUB_DLV_WORKER_LOCK(sub); |
There was a problem hiding this comment.
I missed that in earlier PRs, but for both these APIs, you don't need to call SUB_DLV_WORKER_LOCK, so let's remove.
src/sub.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| const char* subject = NULL; |
There was a problem hiding this comment.
Same, please declare at top, and remove SUB_DLV_WORKER_LOCK/SUB_DLV_WORKER_UNLOCK
kozlovic
changed the title
This change adds the ability to get the subscription id / subject from a given natsSubscription instance.
This is done in a similar manner as the other "get functions" defined in nats.h (e.g. natsSubscription_GetStats which populates various fields such as pending bytes and dropped messages of a given subscription)