fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

david-mackessy · 2025-02-13T15:57:19Z

Issue

The original issue raised was for a GET call to /api/apps/line-listing which resulted in a 404 not found, even though the directory /api/apps/line-listing/ existed and had an index.html file within.

The correct response for a GET call to /api/apps/line-listing (if the directory exists) is a 302 redirect with the existing path e.g. /api/apps/line-listing/ returned as the location header.

Cause

There were multiple issues regarding how we serve app resources:

file system storage behaviour differed from object storage behaviour
different calls returned different results
expectations of the server were not known so expected behaviour was unknown
bundled apps v installed apps behave differently & is a separate issue

Fix

The expectations of the server (app resource rules) were provided by @amcgee. This makes things very clear for all involved (Confluence page exists now). They are:

If the requested path ends with /
a. Respond with contents of ./index.html if it exists
b. Otherwise 404
If the requested path ends with no trailing slash
a. If it exists as a file, return it 200
b. If it exists as a directory, return a 302 redirect to the request path with a / appended
c. otherwise 404

This fix includes:

unifying app resource behaviour, regardless which file storage config is chosen (file system, object).
unifying app resource handling (same code used for each)
removing extra / in places where they were not needed, which resulted in outcomes that were not obvious to the caller

Some clean up was done to use the JClouds feature in the way we probably hoped to use it in that we now handle all app resources the same way. Previously we had different code paths for different storage types which defeats the purpose of using the JClouds abstraction.
The changes were kept to a minimum to reduce the possibility of unseen issues. There are not many automated tests for this feature, given that it requires installing apps and then fetching resources.
There are more object storage tests (using MinIO) than file system tests.

The retrieval results for a Resource have been modelled (ResourceResult) to reflect the rules. This should:

make things clearer
allow easy extension if more result types required
allow high-level code to make decisions based on these values, not having to use nulls or exceptions as a basis for decisions at a high level

App resources are retrieved for both /api/apps/ and /dhis-web-..../ (when installed) endpoints. Both requests come in through different parts of the code. A small change in AppOverrideFilter has been made so that requests to e.g./dhis-web-user-profile/ come through to app resource rules with a resource path of /. Previous behaviour was to pass through an empty resource path. This change means both paths have the same rules applied to the actual resource path requested.

Testing

Automated

Multiple MinIO tests added for all types of expected scenarios
Some AppManager tests added for resource values

Manual

Tested locally & in IM using installed versions of apps (not bundled which has separate behaviour).
Below is a table of the previous behaviour vs new behaviour for different resource requests. Requests not matching the new resource rules are labelled with BUG. They were performed against the following existing directories:

├── test-minio/
│   ├── manifest.webapp
│   ├── index.html
│   ├── subDir/
│   │   ├── index.html
│   │   ├── test-page.html
│   ├── emptyDir/
│   ├── test.dot.dir/
│   │   ├── index.html
│   ├── dir_underscore/
│   │   ├── index.html
├── dhis-web-user-profile/
│   ├── index.html

PREVIOUS BEHAVIOUR

No.	storage	endpoint	response	info
1	local file system	/api/apps/test-minio/index.html	200
2	local file system	/api/apps/test-minio/	200	return index.html
3	local file system	/api/apps/test-minio	404	not found `BUG`
4	local file system	/api/apps/test-minio/subDir/index.html	200
5	local file system	/api/apps/test-minio/subDir/test-page.html	200
6	local file system	/api/apps/test-minio/subDir/	200	return index.html
7	local file system	/api/apps/test-minio/subDir	200	return index.html `BUG`
8	local file system	/api/apps/test-minio/emptyDir	404	not found `BUG`
9	local file system	/api/apps/test-minio/emptyDir/	404	not found
10	local file system	/api/apps/test-minio/test.dot.dir/index.html	200
11	local file system	/api/apps/test-minio/test.dot.dir/	200	returns index.html
12	local file system	/api/apps/test-minio/test.dot.dir	200	returns index.html `BUG`
13	local file system	/api/apps/test-minio/dir_underscore	200	return index.html `BUG`
14	local file system	/api/apps/test-minio/dir_underscore/	200	return index.html
15	local file system	/dhis-web-user-profile/	200	return index.html
16	local file system	/dhis-web-user-profile	301	redirect - separate behaviour
17	object (MinIO)	/api/apps/test-minio/index.html	200
18	object (MinIO)	/api/apps/test-minio/	200	empty body `BUG`
19	object (MinIO)	/api/apps/test-minio	500	error `BUG`
20	object (MinIO)	/api/apps/test-minio/subDir/index.html	200
21	object (MinIO)	/api/apps/test-minio/subDir/test-page.html	200	empty body `BUG`
22	object (MinIO)	/api/apps/test-minio/subDir/	200	empty body `BUG`
23	object (MinIO)	/api/apps/test-minio/subDir	500	error `BUG`
24	object (MinIO)	/api/apps/test-minio/emptyDir	500	error `BUG`
25	object (MinIO)	/api/apps/test-minio/emptyDir/	200	should be 404 `BUG`
26	object (MinIO)	/api/apps/test-minio/test.dot.dir/index.html	200
27	object (MinIO)	/api/apps/test-minio/test.dot.dir/	200	empty body `BUG`
28	object (MinIO)	/api/apps/test-minio/test.dot.dir	500	error `BUG`
29	object (MinIO)	/api/apps/test-minio/dir_underscore	500	error `BUG`
30	object (MinIO)	/api/apps/test-minio/dir_underscore/	200	empty body `BUG`
31	object (MinIO)	/dhis-web-user-profile/	500	error `BUG`
32	object (MinIO)	/dhis-web-user-profile	301	redirect - separate behaviour

NEW BEHAVIOUR

No.	storage	endpoint	response	info
1	local file system	/api/apps/test-minio/index.html	200
2	local file system	/api/apps/test-minio/	200	return index.html
3	local file system	/api/apps/test-minio	302	redirect `FIXED`
4	local file system	/api/apps/test-minio/subDir/index.html	200
5	local file system	/api/apps/test-minio/subDir/test-page.html	200
6	local file system	/api/apps/test-minio/subDir/	200	return index.html
7	local file system	/api/apps/test-minio/subDir	302	redirect `FIXED`
8	local file system	/api/apps/test-minio/emptyDir	302	redirect `FIXED`
9	local file system	/api/apps/test-minio/emptyDir/	404	not found
10	local file system	/api/apps/test-minio/test.dot.dir/index.html	200
11	local file system	/api/apps/test-minio/test.dot.dir/	200	returns index.html
12	local file system	/api/apps/test-minio/test.dot.dir	302	redirect `FIXED`
13	local file system	/api/apps/test-minio/dir_underscore	302	redirect `FIXED`
14	local file system	/api/apps/test-minio/dir_underscore/	200	return index.html `FIXED`
15	local file system	/dhis-web-user-profile/	200	return index.html
16	local file system	/dhis-web-user-profile	301	redirect - separate behaviour
17	object (MinIO)	/api/apps/test-minio/index.html	200
18	object (MinIO)	/api/apps/test-minio/	200	return index.html `FIXED`
19	object (MinIO)	/api/apps/test-minio	302	redirect `FIXED`
20	object (MinIO)	/api/apps/test-minio/subDir/index.html	200
21	object (MinIO)	/api/apps/test-minio/subDir/test-page.html	200	`FIXED`
22	object (MinIO)	/api/apps/test-minio/subDir/	200	return index.html `FIXED`
23	object (MinIO)	/api/apps/test-minio/subDir	302	redirect `FIXED`
24	object (MinIO)	/api/apps/test-minio/emptyDir	302	`FIXED`
25	object (MinIO)	/api/apps/test-minio/emptyDir/	404	`FIXED`
26	object (MinIO)	/api/apps/test-minio/test.dot.dir/index.html	200
27	object (MinIO)	/api/apps/test-minio/test.dot.dir/	200	returns index.html `FIXED`
28	object (MinIO)	/api/apps/test-minio/test.dot.dir	302	redirect `FIXED`
29	object (MinIO)	/api/apps/test-minio/dir_underscore	302	`FIXED`
30	object (MinIO)	/api/apps/test-minio/dir_underscore/	200	return index.html
31	object (MinIO)	/dhis-web-user-profile/	200	return index.html `FIXED`
32	object (MinIO)	/dhis-web-user-profile	301	redirect - separate behaviour

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/AppController.java

david-mackessy · 2025-02-18T13:47:35Z

dhis-2/dhis-support/dhis-support-commons/pom.xml

+    <dependency>
+      <groupId>com.google.code.findbugs</groupId>
+      <artifactId>jsr305</artifactId>
+    </dependency>


the project wouldn't build without this (used undeclared dep etc.)

david-mackessy · 2025-02-18T13:49:06Z

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/AppController.java

-
-      if (resource == null) {
-        response.sendError(HttpServletResponse.SC_NOT_FOUND);
+      ResourceResult resourceResult = appManager.getAppResource(application, resource);


these results can be handled more nicely when we move to the higher Java version with switch pattern matching

david-mackessy · 2025-02-18T13:51:05Z

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/filter/AppOverrideFilter.java

      }
-      response.setHeader("ETag", etag);
-      StreamUtils.copyThenCloseInputStream(resource.getInputStream(), response.getOutputStream());
+      if (resourceResult instanceof Redirect redirect) {


There were no existing redirect code paths here, so now this has to be handled as a possible result.

david-mackessy · 2025-02-19T11:14:43Z

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/filter/AppOverrideFilter.java

+        // this is due to the complex regex above which has to include '/' at the end, so correct
+        // app names are matched e.g. dhis-web-user v dhis-web-user-profile
+        serveInstalledAppResource(
+            app, resourcePath.isBlank() ? "/" : resourcePath, request, response);


this change ensures that the actual resource requested e.g. dhis-web-user-profile/ is correctly matched with resource rule 1a.

dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/ResourceResult.java

...-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/util/TextUtils.java

amcgee

Great work @david-mackessy, thanks for following this through so thoroughly!

It would be great if @cjmamo and @KaiVandivier could take a look at the code and logic as well before this is merged (mostly to be aware of it, since we'll be looking at general improvements to this app serving infrastructure soon)

dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/AppStorageService.java

...-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/util/TextUtils.java

...vices/dhis-service-core/src/main/java/org/hisp/dhis/appmanager/JCloudsAppStorageService.java

sonarqubecloud · 2025-02-19T15:42:54Z

Quality Gate passed

Issues
0 New issues
5 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

david-mackessy added 10 commits February 10, 2025 08:21

fix: Return index.html when object storage used with an empty page name

6b0f602

update tests

96c2a8b

Merge branch 'master' into obj-storage-index-html

6a4e772

fix: Handle multiple resource variations with tests

d6c79f8

fix: Clean up var names and test resource

7fbf6c0

format

dfe8c7b

Merge branch 'obj-storage-index-html' into DHIS2-18185

7b31fc5

fix: Unify serving app resources, fixing multiple issues [DHIS2-18185]

927e83a

fix: Update test app zip

0e382db

Merge branch 'master' into DHIS2-18185

87165d3

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/AppController.java Dismissed Show dismissed Hide dismissed

david-mackessy added 9 commits February 13, 2025 16:18

fix: Clean up

e1cc4a7

fix: Remove tests that won't work in pipeline, file system

5dccafd

fix: Clean up

4dd3726

fix: Move resource result types into 1 file

bbbc4df

fix: Add redirect result in app override filter

4eeeabf

fix: Update debug logs

f48a3f1

fix: Update log format

a5e08cc

fix: Base app dir with no slash is redirect

5fb46f3

Merge branch 'master' into DHIS2-18185

adf4240

david-mackessy changed the title ~~Dhis2 18185~~ fix: Serving App resource rules [DHIS2-18185] Feb 18, 2025

fix: Move utils method to existing class

27e4b7d

david-mackessy commented Feb 18, 2025

View reviewed changes

david-mackessy added 4 commits February 19, 2025 08:24

fix: App base calls without slash are redirected

95a4901

fix: App base calls without slash are redirected to correct path

eadfae0

fix: Allow dhis-web resource path '/' through for rule application

a65309a

Merge branch 'master' into DHIS2-18185

729cf7d

david-mackessy commented Feb 19, 2025

View reviewed changes

fix: Remove redundant code and extract method for better reading

5c646d7

david-mackessy changed the title ~~fix: Serving App resource rules [DHIS2-18185]~~ fix: Serving App resource rules (installed apps) [DHIS2-18185] Feb 19, 2025

david-mackessy marked this pull request as ready for review February 19, 2025 14:58

david-mackessy requested a review from amcgee February 19, 2025 15:02

jbee approved these changes Feb 19, 2025

View reviewed changes

dhis-2/dhis-api/src/main/java/org/hisp/dhis/appmanager/ResourceResult.java Outdated Show resolved Hide resolved

...-2/dhis-support/dhis-support-commons/src/main/java/org/hisp/dhis/commons/util/TextUtils.java Outdated Show resolved Hide resolved

amcgee requested a review from cjmamo February 19, 2025 15:23

amcgee approved these changes Feb 19, 2025

View reviewed changes

david-mackessy added 2 commits February 19, 2025 15:37

fix: Remove permits and use string replace all

b7f4dd5

Merge branch 'master' into DHIS2-18185

b06a5c8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

david-mackessy commented Feb 13, 2025 •

edited

Loading

david-mackessy Feb 18, 2025

david-mackessy Feb 18, 2025

david-mackessy Feb 18, 2025

david-mackessy Feb 19, 2025

amcgee left a comment

sonarqubecloud bot commented Feb 19, 2025 •

edited

Loading

fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

Are you sure you want to change the base?

fix: Serving App resource rules (installed apps) [DHIS2-18185] #19923

Conversation

david-mackessy commented Feb 13, 2025 • edited Loading

Issue

Cause

Fix

Testing

Automated

Manual

PREVIOUS BEHAVIOUR

NEW BEHAVIOUR

david-mackessy Feb 18, 2025

Choose a reason for hiding this comment

david-mackessy Feb 18, 2025

Choose a reason for hiding this comment

david-mackessy Feb 18, 2025

Choose a reason for hiding this comment

david-mackessy Feb 19, 2025

Choose a reason for hiding this comment

amcgee left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 19, 2025 • edited Loading

Quality Gate passed

david-mackessy commented Feb 13, 2025 •

edited

Loading

sonarqubecloud bot commented Feb 19, 2025 •

edited

Loading