Skip to content
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

feat(9586): implement freetext search in cht datasource #9625

Open
wants to merge 53 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
ac0ab67
Initial setup
sugat009 Nov 6, 2024
cd11adc
add /api/v1/contacts/:uuid endpoint
sugat009 Nov 7, 2024
b7c5d1f
add /api/v1/contact/:uuid?with_lineage=<option> endpoint
sugat009 Nov 7, 2024
f91e393
add missing files
sugat009 Nov 7, 2024
52ac11b
add missing files
sugat009 Nov 7, 2024
c1b68cf
add endpoint /api/v1/report
sugat009 Nov 8, 2024
45c0ccf
add additional checks for report validity
sugat009 Nov 11, 2024
acf903a
add /api/contact/id endpoint (not tested yet)
sugat009 Nov 12, 2024
89927b1
add endpoint /api/v1/report/id --untested
sugat009 Nov 13, 2024
45abf58
implement search feature in /api/contact/id endpoint
sugat009 Nov 15, 2024
6f5c326
add search functionality to /api/report/id endpoint
sugat009 Nov 15, 2024
192af16
add async API Contact.v1.getIdsAll
sugat009 Nov 15, 2024
bb98dcf
add async API Report.v1.getIds
sugat009 Nov 15, 2024
43efbef
add JSDocs for functions
sugat009 Nov 18, 2024
ede85fd
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 18, 2024
2b33c07
add unit tests for index.ts, qualifier.ts and contact.ts
sugat009 Nov 20, 2024
3bfcace
add unit tests for report.ts
sugat009 Nov 20, 2024
074b0c1
add tests for local/contact.js
sugat009 Nov 25, 2024
1907907
add some additional tests in local/contact.spec.ts
sugat009 Nov 25, 2024
a629116
add tests for local/report.ts
sugat009 Nov 25, 2024
c45cee8
add unit tests for remote/contact.ts
sugat009 Nov 25, 2024
5400736
add unit tests for remote/report.ts
sugat009 Nov 25, 2024
bf46e4a
add unit tests for contact-types.ts
sugat009 Nov 25, 2024
b1cf669
remove unused variables
sugat009 Nov 25, 2024
98fd4e2
add unit tests for api/src/controllers/contact.js
sugat009 Nov 26, 2024
ee32262
add tests for api/src/controllers/report.js
sugat009 Nov 26, 2024
b5bc6e6
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 27, 2024
4a18fc8
Merge remote-tracking branch 'origin/master' into 9586-implement-free…
sugat009 Nov 27, 2024
eba1b61
add integration test for api/src/controller/contact.js
sugat009 Nov 29, 2024
9157059
add integration tests for api/src/controllers/report.js
sugat009 Nov 29, 2024
d5fed9c
fix eslint issues
sugat009 Nov 29, 2024
30d058e
changes missed during copy-paste
sugat009 Dec 2, 2024
662598c
change title of tests
sugat009 Dec 2, 2024
89eb816
fix indentation
sugat009 Dec 2, 2024
3ea31ef
fix max-line 120 issue
sugat009 Dec 2, 2024
a7a9272
rename getIdsAll functions to getIds
sugat009 Dec 3, 2024
ef8b93f
add tests in controller unit tests for when limit is undefined or null
sugat009 Dec 3, 2024
43c3896
add page limits and end of alphabet marker to separate constants file
sugat009 Dec 4, 2024
f97f928
add function in qualifier.ts to check for key-value based freetext qu…
sugat009 Dec 4, 2024
2065125
separate out the endpoints integration tests and cht-datasource integ…
sugat009 Dec 5, 2024
f8087b9
remove unused variables
sugat009 Dec 5, 2024
e6aea4f
move src/constants.ts to src/libs/constants.ts
sugat009 Dec 5, 2024
c21e52a
change limit to number and stringified number in cht-datasource
sugat009 Dec 6, 2024
c487a7e
Update shared-libs/cht-datasource/src/qualifier.ts
sugat009 Dec 23, 2024
ce1013a
Update shared-libs/cht-datasource/test/contact.spec.ts
sugat009 Dec 23, 2024
19143b4
Update shared-libs/cht-datasource/src/remote/contact.ts
sugat009 Dec 24, 2024
8dc7de3
addressing PR review comments
sugat009 Dec 24, 2024
91a0125
Merge branch '9586-implement-freetext-search-in-cht-datasource' of gi…
sugat009 Dec 24, 2024
49553b7
address most of the comments
sugat009 Dec 30, 2024
22da4b9
fix integration tests
sugat009 Dec 30, 2024
4a0d2bb
fix api unit tests
sugat009 Dec 31, 2024
bfa0a57
refactor getWithLineage contacts logic
sugat009 Jan 3, 2025
eab5877
listen to eslint
sugat009 Jan 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions api/src/controllers/contact.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const auth = require('../auth');
const { Contact, Qualifier } = require('@medic/cht-datasource');
const ctx = require('../services/data-context');
const serverUtils = require('../server-utils');

const getContact = ({ with_lineage }) => ctx.bind(with_lineage === 'true' ? Contact.v1.getWithLineage : Contact.v1.get);
const getContactIds = () => ctx.bind(Contact.v1.getIdsPage);

const checkUserPermissions = async (req) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
};

module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);
const { uuid } = req.params;
const contact = await getContact(req.query)(Qualifier.byUuid(uuid));

if (!contact) {
return serverUtils.error({ status: 404, message: 'Contact not found' }, req, res);
}

return res.json(contact);
}),
getIds: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);

if (!req.query.freetext && !req.query.type) {
return serverUtils.error({ status: 400, message: 'Either query param freetext or type is required' }, req, res);
}
const qualifier = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in an earlier comment, ideally we should be able to use a Qualifier.and function to combine qualifier objects. Something like:

const getSearchQualifier = (freetext, type) => {
  if (freetext && type) {
    return Qualifier.and(Qualifier.byFreetext(freetext), Qualifier.byContactType(type));
  }
  if (freetext) {
    return Qualifier.byFreetext(freetext);
  }
  if (type) {
    return Qualifier.byContactType(type);
  }

  const err = new Error('Either query param freetext or type is required');
  err.status = 400;
  throw err;
};


if (req.query.freetext) {
Object.assign(qualifier, Qualifier.byFreetext(req.query.freetext));
}

if (req.query.type) {
Object.assign(qualifier, Qualifier.byContactType(req.query.type));
}

const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;
const limit = req.query.limit ? Number(req.query.limit) : false;

The false is a random pick.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkuester thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sugat009 sugat009 Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jkuester jkuester Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 😅 😅 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const docs = await getContactIds()(qualifier, req.query.cursor, limit);

return res.json(docs);
}),
},
};
40 changes: 40 additions & 0 deletions api/src/controllers/report.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
const auth = require('../auth');
const ctx = require('../services/data-context');
const serverUtils = require('../server-utils');
const { Report, Qualifier } = require('@medic/cht-datasource');

const getReport = () => ctx.bind(Report.v1.get);
const getReportIds = () => ctx.bind(Report.v1.getIdsPage);

const checkUserPermissions = async (req) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
};

module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await checkUserPermissions(req);
const { uuid } = req.params;
const report = await getReport()(Qualifier.byUuid(uuid));

if (!report) {
return serverUtils.error({ status: 404, message: 'Report not found' }, req, res);
}

return res.json(report);
}),
getIds: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);

const qualifier = Qualifier.byFreetext(req.query.freetext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;

const docs = await getReportIds()(qualifier, req.query.cursor, limit);

return res.json(docs);
})
}
};
8 changes: 8 additions & 0 deletions api/src/routing.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ const exportData = require('./controllers/export-data');
const records = require('./controllers/records');
const forms = require('./controllers/forms');
const users = require('./controllers/users');
const contact = require('./controllers/contact');
const person = require('./controllers/person');
const place = require('./controllers/place');
const report = require('./controllers/report');
const { people, places } = require('@medic/contacts')(config, db, dataContext);
const upgrade = require('./controllers/upgrade');
const settings = require('./controllers/settings');
Expand Down Expand Up @@ -492,6 +494,12 @@ app.postJson('/api/v1/people', function(req, res) {
app.get('/api/v1/person', person.v1.getAll);
app.get('/api/v1/person/:uuid', person.v1.get);

app.get('/api/v1/contact/id', contact.v1.getIds);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

app.get('/api/v1/contact/:uuid', contact.v1.get);

app.get('/api/v1/report/id', report.v1.getIds);
app.get('/api/v1/report/:uuid', report.v1.get);

app.postJson('/api/v1/bulk-delete', bulkDocs.bulkDelete);

// offline users are not allowed to hydrate documents via the hydrate API
Expand Down
Loading
Loading