Skip to content

Initial work to support CRUD through REST#26

Draft
ojwanganto wants to merge 4 commits into
openmrs:masterfrom
palladiumkenya:feat/add-rest-endpoint
Draft

Initial work to support CRUD through REST#26
ojwanganto wants to merge 4 commits into
openmrs:masterfrom
palladiumkenya:feat/add-rest-endpoint

Conversation

@ojwanganto
Copy link
Copy Markdown

No description provided.

import java.util.Date;
import java.util.Map;

public class DataFilterDefaultResponse {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please build atop the REST web services module’s API to maintain uniformity from a client perspective.

import java.util.Map;
import java.util.stream.Collectors;

@Component
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Like I said earlier, build on top of the REST API instead of creating this logic from scratch.

if (basis == null) {
return new ResponseEntity<>("Data filter Basis is required!", HttpStatus.BAD_REQUEST);
}
dataFilterService.grantAccess(entity, basis);
Copy link
Copy Markdown
Member

@wluyima wluyima Jul 4, 2024

Choose a reason for hiding this comment

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

Methods like grantAccess are not protected in the Java API, this will be a security hole because anyone can call this endpoint to grant themselves access to forbidden resources.


@RequestMapping(method = RequestMethod.POST, value = "search")
@ResponseBody
public List<DataFilterDefaultResponse> searchEntityBasisMap(@Valid @RequestBody EntityBasisMapSearchRequest searchQuery)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be a protected resource that you don't want every user to view.

@@ -0,0 +1,93 @@
-- create a mapping:
Copy link
Copy Markdown
Member

@wluyima wluyima Jul 4, 2024

Choose a reason for hiding this comment

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

We would need to return resources based on the web services module's API, let's not create 'deviant' resources.

Copy link
Copy Markdown
Member

@wluyima wluyima left a comment

Choose a reason for hiding this comment

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

I have added some comments, let's have a ticket and discuss before implementing this.

@ojwanganto
Copy link
Copy Markdown
Author

Thanks, @wluyima. Comments well noted

@ibacher
Copy link
Copy Markdown
Member

ibacher commented Jul 5, 2024

I agree with @wluyima's points. At a slight step back, I wonder if it might make sense to move the Location-based filtering and the REST API here into a separate, optional JAR? The issue is that the REST API here is only useful with location-based filter stuff, but that isn't the only thing that this module is able to filter. Basically, I'd like some kind of "data filter core" with a "location-based limit" addon for that core, if that makes sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants