I'm not a big fan of this callback style.

We've been doing this pattern for all the REST endpoints that call cht-datasource. What's the alternative?

IMHO doOrError is a nice way to reduce duplicated code and ensure we are handling errors consistently.

I think a try-catch block is not so much duplication, and it's more transparent than a nested callback.
I understand this already exists. I'm not a fan.

this seems strange that we assign a random non-truthy value (as in: whatever is in req.query.limit) instead of being specific.
Same applies to the reports controller.

The false is a random pick.

The reason why req.query.limit is being passed when the conditional is falsy is that in the cht-datasource there is already a validation for this and also a default value. This is to ensure that the validation does not happen twice and also the default value.
Reference.

Then why not have cht-datasource also do the Number conversion then? Why have this validation here? Is limit ever expected to not be a number?

Is limit ever expected to not be a number?

Yeah, in cases like the one above, where it is passed as a query param in REST API, it is expected to be a stringified number. However, cht-datasource can also be used in non-REST API codes where end-users will have to pass in a number to cht-datasource because PouchDB expects the limit value to be a number. I think that's a reasonable approach to make the limit variable an explicit Number type, as that would align with the expected input for the PouchDB Adapter. This would provide better type safety and clarity in the code. The validation being present in cht-datasource still makes sense, and whether to apply the same validation elsewhere should be at the discretion of the end-user, based on their specific use case.

So even if it's a stringified number or a number, we still only ever evaluate it as a number. so it makes sense to only have validation in one spot, right?

@jkuester thoughts on this?

Looking at this again, I cannot see any reason why it would be a problem to just directly pass req.query.limit to cht-datasource. We already thoroughly validate the limit argument in the cht-datasource logic and JS is not going to have any issue auto-boxing any valid number (without needing to be wrapped in Number())....

In JS, the query parameters are almost all strings and when they are passed into other functions or classes, will still be strings. I've tried this before. Here.

😅 😅 😅 I knew we had discussed this before. I was not able to find that thread you linked to and so I proceeded to repeat the exact same line of reasoning that lead me to start that thread in the first place... 🤦

sigh

Now I think I am 100% following what you are saying above and agree with this statement:

I'm not sure why the conversion of limit from string to number should be designated to cht-datasource, it's not its concern.

The cht-datasource apis are responsible for sanitizing the input to ensure it confirms to the specified expectations. Generally speaking, I do not think cht-datasource should need to include extra logic to "support" different ways that consumers decide to provide data (accidentally or intentionally). The type number | string does not precisely communicate the valid range of values for limit (which should only ever be numeric).

That being said, the cht-datasource interfaces should be designed to be convenient to consume as long as it does not compromise the clarity of the API. It turns out that TypeScript has a type that represents "a string containing a number value": `${number}`! This means that we could specify the type of limit to be number | `${number}` and then handle converting any string to an actual number just inside the edge of the cht-datasource code flow. To me, that seems like the best of both worlds where we are not always fighting the weird non-auto-boxing behavior when calling cht-datasource from JS code, but at the same time, our TS APIs do not have to be unnecessarily promiscuous. Does that make sense?

This means that we could specify the type of limit to be number | ${number}

Sounds good. As long as we don't have logic around limit in multiple places.
But, philosophically, I don't think a programming languages (typescript in this case) should dictate how software is written, and even if there wasn't a type, then we should still have the option to design software however we want.

Please make the change.

So ... this endpoint .. if it doesn't get neither a freetext query param or a limit query param, it will end up returning ALL reports?

nope. it returns a 400 - Bad request error because freetext is required whereas limit is set to a default of 10000.

maybe /api/v1/contact/ids is more suitable.
The idea is that the URL isn't suggestive at all, without reading the implementation, I would never guess what this endpoint does.

Yes, REST API conventions are to name the API endpoint in a plural way like /api/v1/contacts or /api/v1/contacts/ids but this design decision had already been taken even before this ticket. I couldn't find the link to the conversation for this though.

That discussion happened in the parent ticket before we spun off the child isssue: #9544 (comment)

Thanks @jkuester . Your argument here is that "we've already decided and your input is not welcome?"

Sorry for being aggressive and confrontational in the above comment.

I maintain my comment about /api/v1/contact/id being quite unsuggestive, we shouldn't need thorough explanations and reasoning behind the naming choice in order for an api name to make sense.

I was just trying to provide the context for the discussion that Sugat referenced. 😬

I am happy to continue the design discussion here to come to an agreed upon approach. It will just be most efficient if we all understand what was already said to get us here. When starting work on new REST endpoints for the cht-datasource code, we chose to go with the pattern of singular entity names (so /api/v1/person instead of /api/v1/persons). When the endpoint can return 0-n entities I do not really see a compelling reason to prefer either singular or plural (since either might make more sense depending on the context). Two things seem clear to me though:

• Under normal circumstances, we should not duplicate endpoints for the same resource (e.g. having both /api/v1/contact/id and /api/v1/contact/ids).
• We should be consistent with our naming across our go-forward REST endpoints. Either using singular or plural, but not mixing both.

I agree with being consistent.

I personally can't recall seeing APIs in the world that used this singular form, so for me this seems quite unintuitive.

Same comment about assertions in afterEach and afterEach run order.