fix: Move TE request validations to appropiate layers [TECH-1656] #15732

muilpp · 2023-11-17T12:01:22Z

We had a few validations regarding tracked entities that were scattered around the service, outside the mapper. They were mainly decideAccess(), validate(), and validateSearchScope().
Now the three of them are split and all the contained validations are in the mapper.

Ticket: https://dhis2.atlassian.net/browse/TECH-1656

…ion-layer

codecov · 2023-11-21T14:53:47Z

Codecov Report

Merging #15732 (30c26f4) into master (a3ecb7a) will increase coverage by 0.01%.
The diff coverage is 52.02%.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15732      +/-   ##
============================================
+ Coverage     66.33%   66.34%   +0.01%     
+ Complexity    31430    31429       -1     
============================================
  Files          3489     3489              
  Lines        130103   130091      -12     
  Branches      15201    15191      -10     
============================================
+ Hits          86302    86312      +10     
+ Misses        36718    36700      -18     
+ Partials       7083     7079       -4

Flag	Coverage Δ
integration	`50.04% <41.21%> (+<0.01%)`	⬆️
integration-h2	`32.34% <25.67%> (-0.01%)`	⬇️
unit	`30.40% <37.16%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...dhis/tracker/export/OperationsParamsValidator.java	`66.66% <100.00%> (+4.76%)`	⬆️
...ort/trackedentity/DefaultTrackedEntityService.java	`84.39% <ø> (+19.30%)`	⬆️
...export/trackedentity/TrackedEntityQueryParams.java	`86.23% <100.00%> (ø)`
...rackedentity/TrackedEntityRequestParamsMapper.java	`79.43% <25.00%> (-19.30%)`	⬇️
...ckedentity/TrackedEntityOperationParamsMapper.java	`72.55% <57.62%> (-18.62%)`	⬇️

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3ecb7a...30c26f4. Read the comment docs.

...his/webapi/controller/tracker/export/trackedentity/TrackedEntityRequestParamsMapperTest.java

…ion-layer

...bapi/controller/tracker/export/trackedentity/TrackedEntityImportRequestParamsMapperTest.java

…ion-layer

...bapi/controller/tracker/export/trackedentity/TrackedEntityImportRequestParamsMapperTest.java

+  @Test
+  void shouldMapCorrectlyWhenTrackedEntityAndSpecificUpdatedRangeSupplied()
+      throws BadRequestException {
+    trackedEntityRequestParams.setOuMode(CAPTURE);


muilpp · 2023-11-27T12:44:41Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

@@ -66,22 +75,36 @@ class TrackedEntityOperationParamsMapper {

  @Nonnull private final TrackedEntityAttributeService attributeService;

+  @Nonnull private final AclService aclService;
+
+  @Nonnull private final TrackedEntityStore trackedEntityStore;


I don't like having the store dependency in the mapper, but this is meant to be a temporary solution until we work on https://dhis2.atlassian.net/browse/DHIS2-15915

I agree with you.
Can we at least add a comment or a TODO to it?

enricocolasante

Most of the comments are in fact on the old code that was just moved somewhere else but I think that now is the moment to clean up that code too.

The check removed for F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS seems a big deal to me, I am not sure if it is supposed to be in this PR

enricocolasante · 2023-11-27T13:09:09Z

...ice-tracker/src/main/java/org/hisp/dhis/tracker/export/event/EventOperationParamsMapper.java

-    if (user == null
-        || user.isSuper()
-        || user.isAuthorized(F_TRACKED_ENTITY_INSTANCE_SEARCH_IN_ALL_ORGUNITS)) {
+    if (user == null || user.isSuper()) {


This seems like a bug fixing.
Should it be here in this PR?

I can create a new ticket and add it there

I think so because this probably needs to be backported, right?

it's possible yes

...sp/dhis/webapi/controller/tracker/export/trackedentity/TrackedEntityRequestParamsMapper.java

enricocolasante · 2023-11-27T13:15:55Z

...sp/dhis/webapi/controller/tracker/export/trackedentity/TrackedEntityRequestParamsMapper.java

+    if (params.getProgram() == null) {
+      if (params.getProgramStatus() != null) {
+        violation = "Program must be defined when program status is defined";
+      }
+
+      if (params.getFollowUp() != null) {
+        violation = "Program must be defined when follow up status is defined";
+      }
+
+      if (params.getEnrollmentEnrolledAfter() != null) {
+        violation = "Program must be defined when program enrollment start date is specified";
+      }
+
+      if (params.getEnrollmentEnrolledBefore() != null) {
+        violation = "Program must be defined when program enrollment end date is specified";
+      }
+
+      if (params.getEnrollmentOccurredAfter() != null) {
+        violation = "Program must be defined when program incident start date is specified";
+      }
+
+      if (params.getEnrollmentOccurredBefore() != null) {
+        violation = "Program must be defined when program incident end date is specified";
+      }
+    }
+
+    if (params.getEventStatus() != null
+        && (params.getEventOccurredAfter() == null || params.getEventOccurredBefore() == null)) {
+      violation = "Event start and end date must be specified when event status is specified";
+    }
+
+    if (params.getUpdatedWithin() != null
+        && (params.getUpdatedAfter() != null || params.getUpdatedBefore() != null)) {
+      violation =
+          "Last updated from and/or to and last updated duration cannot be specified simultaneously";
+    }


Can you review the naming here?
For example, we don't have incident date anymore and last updated is just called updated now.

I'm reading the release notes, I see in the DB, the column name incidentdate is now called ocurreddate, but I don't see any mention to the parameters used in the payload. Is this explained somewhere else?

The parameters used in the payload didn't change so they are not in the release notes.
Here it is just a matter to check how things are called in the if statement and change the error message accordly.
I would even use a notation with back quotes to highlight the parameters that needs to be corrected

enricocolasante · 2023-11-27T13:28:57Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

+  private List<TrackedEntityType> filterAndValidateTrackedEntityTypes(
+      User user, TrackedEntityType trackedEntityType, Program program) throws BadRequestException {
+    List<TrackedEntityType> trackedEntityTypes = new ArrayList<>();
+    if (trackedEntityType == null) {


If program is not null, should we still define the trackedEntityTypes?

when the program is not null we still need to do it, but that's a good point, because we don't need to do it if a trackedEntityType is present in the request.

If Program is not null, shouldn't be somoething like this?
trackedEntityTypes = List.of(program.getTrackedEntityType)

I did it this way because it's how it's done before this refactor.
When no trackedEntityType is specified, it gets all the ones accessible by the user. Is there a reason why we should use the one of the program instead?

https://github.com/dhis2/dhis2-core/blob/master/dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/trackedentity/DefaultTrackedEntityService.java#L472

enricocolasante · 2023-11-27T13:35:14Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

@@ -66,22 +75,36 @@ class TrackedEntityOperationParamsMapper {

  @Nonnull private final TrackedEntityAttributeService attributeService;

+  @Nonnull private final AclService aclService;
+
+  @Nonnull private final TrackedEntityStore trackedEntityStore;


I agree with you.
Can we at least add a comment or a TODO to it?

…ion-layer

muilpp · 2023-11-28T12:07:40Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

+            .toList();
+
+    if (program == null && trackedEntityTypes.isEmpty()) {
+      throw new BadRequestException("User has no access to any Tracked Entity Type");


Not sure if this is correct. Need to check if the query can return results if program is null and user has no access to any tet. In that case, the message will still be Either a program or tracked entity type must be specified

A TrackedEntity must have a TrackedEntityType even if it is not enforced at DB level.
So it is correct to stop the user to get a collection of TEs if it has no access to any TrackedEntityType

It's also possible to run a request if trackedEntities is present, also when program and trackedEntityType are null.

enricocolasante · 2023-11-28T13:03:47Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

+            .toList();
+
+    if (program == null && trackedEntityTypes.isEmpty()) {
+      throw new BadRequestException("User has no access to any Tracked Entity Type");


A TrackedEntity must have a TrackedEntityType even if it is not enforced at DB level.
So it is correct to stop the user to get a collection of TEs if it has no access to any TrackedEntityType

enricocolasante · 2023-11-28T13:04:44Z

...sp/dhis/webapi/controller/tracker/export/trackedentity/TrackedEntityRequestParamsMapper.java

+
+    if (params.getProgram() != null && params.getTrackedEntityType() != null) {
+      throw new BadRequestException(
+          "Program and tracked entity cannot be specified simultaneously");


Suggested change

"Program and tracked entity cannot be specified simultaneously");

"Program and tracked entity type cannot be specified simultaneously");

…ion-layer

enricocolasante · 2023-11-29T08:18:36Z

...main/java/org/hisp/dhis/tracker/export/trackedentity/TrackedEntityOperationParamsMapper.java

+    return emptyList();
+  }
+
+  private List<TrackedEntityType> filterAndValidateTrackedEntityTypes(User user) {


I still think we should validate if user has no access to any Tracked Entity Types because if we skip this validation in the case the client specifies something in trackedEntities then we could return some results even if the user has no access to.

When we run the query against the DB we join the tracked entity type table and we filter by the value we found here. In case there's no value, the result of the query will be an empty list, so in theory we should not leak any information. But I agree that it's better to stop the request here instead of running a query against a database when we already know we'll return an empty list.

…ies-operation-layer' into TECH-1656-refactor-tracked-entities-operation-layer

…ion-layer

sonarqubecloud · 2023-11-29T18:57:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

muilpp and others added 7 commits November 17, 2023 13:00

fix: Move TE request validations to appropiate layers [TECH-1656]

04b8c97

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

49d2195

…ion-layer

fix: Refactor tracked entities tests [TECH-1656]

0b0ea1a

fix: Refactor tracked entities tests [TECH-1656]

6ca1dfb

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

7e68df7

…ion-layer

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

60a1540

…ion-layer

fix: Split general TE request mapping test in two [TECH-1656]

0caf5cd

github-advanced-security bot found potential problems Nov 21, 2023

View reviewed changes

...his/webapi/controller/tracker/export/trackedentity/TrackedEntityRequestParamsMapperTest.java Fixed Show fixed Hide fixed

muilpp and others added 4 commits November 23, 2023 08:11

fix: Merge validateUser method into existing ones [TECH-1656]

0b2d496

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

6d6f11c

…ion-layer

fix: Refactor trackedEntities tests [TECH-1656]

40eb2d3

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

f4cb1dd

…ion-layer

github-advanced-security bot found potential problems Nov 27, 2023

View reviewed changes

...bapi/controller/tracker/export/trackedentity/TrackedEntityImportRequestParamsMapperTest.java Fixed Show fixed Hide fixed

muilpp and others added 5 commits November 27, 2023 12:11

fix: Refactor global search validation [TECH-1656]

aee19d5

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

b7d7f7a

…ion-layer

fix: Format code [TECH-1656]

7aa0da9

fix: Format code [TECH-1656]

33b3457

fix: Fix broken test after refactor [TECH-1656]

3c8b740

github-advanced-security bot found potential problems Nov 27, 2023

View reviewed changes

fix: Removed TODO comment [TECH-1656]

dddc159

muilpp commented Nov 27, 2023

View reviewed changes

muilpp marked this pull request as ready for review November 27, 2023 12:55

muilpp requested review from enricocolasante, abyot, lucaCambi77 and zubaira November 27, 2023 12:55

enricocolasante reviewed Nov 27, 2023

View reviewed changes

muilpp added 2 commits November 27, 2023 15:53

fix: Minor refactor after PR [TECH-1656]

a5e1e57

fix: Minor refactor after PR [TECH-1656]

394858c

muilpp and others added 2 commits November 27, 2023 16:42

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

75094b4

…ion-layer

fix: Update error messages according to new params naming [TECH-1656]

1a96c81

muilpp requested a review from enricocolasante November 28, 2023 10:21

muilpp and others added 3 commits November 28, 2023 12:27

fix: Use program tet if available [TECH-1656]

f1f27b6

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

1e3676d

…ion-layer

fix: Fix test error message [TECH-1656]

67694e2

muilpp marked this pull request as draft November 28, 2023 12:05

muilpp commented Nov 28, 2023

View reviewed changes

enricocolasante approved these changes Nov 28, 2023

View reviewed changes

muilpp and others added 4 commits November 28, 2023 21:11

fix: Add validation to have program or tei info in request [TECH-1656]

fb131da

fix: Remove exception when fetching tracked entity types [TECH-1656]

6d27ff3

fix: Remove exception when fetching tracked entity types [TECH-1656]

14a745d

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

78869fd

…ion-layer

enricocolasante reviewed Nov 29, 2023

View reviewed changes

muilpp and others added 7 commits November 29, 2023 09:19

fix: Use correct value of trackedEntities on validation [TECH-1656]

e14585f

Merge remote-tracking branch 'origin/TECH-1656-refactor-tracked-entit…

3e73375

…ies-operation-layer' into TECH-1656-refactor-tracked-entities-operation-layer

fix: Throw exception if user has no access to any TET [TECH-1656]

ece5ee5

fix: Add TET to program integration test TET [TECH-1656]

bdeecde

fix: Add test to validate exception when no TET found [TECH-1656]

25b759c

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

f93c395

…ion-layer

Merge branch 'master' into TECH-1656-refactor-tracked-entities-operat…

30c26f4

…ion-layer

muilpp marked this pull request as ready for review November 29, 2023 18:54

muilpp enabled auto-merge (squash) November 29, 2023 18:54

enricocolasante disabled auto-merge November 30, 2023 07:43

enricocolasante merged commit 20bb935 into master Nov 30, 2023
18 checks passed

enricocolasante deleted the TECH-1656-refactor-tracked-entities-operation-layer branch November 30, 2023 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Move TE request validations to appropiate layers [TECH-1656] #15732

fix: Move TE request validations to appropiate layers [TECH-1656] #15732

muilpp commented Nov 17, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading

muilpp Nov 27, 2023

enricocolasante Nov 27, 2023

enricocolasante left a comment

enricocolasante Nov 27, 2023

muilpp Nov 27, 2023

enricocolasante Nov 27, 2023

muilpp Nov 27, 2023

enricocolasante Nov 27, 2023

muilpp Nov 27, 2023

enricocolasante Nov 28, 2023

enricocolasante Nov 27, 2023

muilpp Nov 27, 2023

enricocolasante Nov 28, 2023

muilpp Nov 28, 2023

enricocolasante Nov 27, 2023

muilpp Nov 28, 2023

enricocolasante Nov 28, 2023

muilpp Nov 28, 2023

enricocolasante Nov 28, 2023

enricocolasante Nov 28, 2023

enricocolasante Nov 29, 2023

muilpp Nov 29, 2023

sonarqubecloud bot commented Nov 29, 2023

	"Program and tracked entity cannot be specified simultaneously");
	"Program and tracked entity type cannot be specified simultaneously");

fix: Move TE request validations to appropiate layers [TECH-1656] #15732

fix: Move TE request validations to appropiate layers [TECH-1656] #15732

Conversation

muilpp commented Nov 17, 2023 • edited Loading

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enricocolasante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 29, 2023

muilpp commented Nov 17, 2023 •

edited

Loading

codecov bot commented Nov 21, 2023 •

edited

Loading