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

Initial work to support CRUD through REST #26

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ojwanganto
Copy link

No description provided.

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

public class DataFilterDefaultResponse {
Copy link
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
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
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
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
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
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
Author

Thanks, @wluyima. Comments well noted

@ibacher
Copy link
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