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

fix frontendproperties readFile() to work with classpath and update download_custom_buttons_json reference #10946

Merged
merged 14 commits into from
Aug 22, 2024

Conversation

pappde
Copy link
Contributor

@pappde pappde commented Aug 16, 2024

This fixes the readFile() method which is used by frontendproperties (application.properties) to read the contents of a file in, to work with the "classpath:" syntax. This PR also updates the reference config for download_custom_buttons_json to the classpath syntax so it works.

Properties affected:

  • frontendConfigOverride
  • query_sets_of_genes
  • skin.patient_view.custom_sample_type_colors_json
  • oncoprint.clinical_tracks.config_json

Explanation

The existing documentation for these properties mentioned that "classpath:" syntax could be used. E.g. from the application.properties.EXAMPLE, I don't believe this would work:

DOES NOT WORK
oncoprint.clinical_tracks.config_json=classpath:/oncoprint-default-tracks.json

However, the readFile() method didn't support that syntax. It would only work with the file system. Furthermore, production instances (at least the public cBioPortal) build the server to a single "app.jar" file (using the "web" docker config). If the above file exists in the "web-and-data" loose configuration via the repo, but no longer exists as a loose file in the "web" configuration, then you have configuration that works in dev but fails in production.

WORKS IN DEV BUT NOT PRODUCTION
download_custom_buttons_json=/cbioportal-webapp/custom_buttons/download_custom_button_avm.json

That means that these properties only worked if they had an absolute path reference to a file, and the file is manually placed on the file system of the server (and not packed into the app.jar). E.g.

WORKS IF THE FILE IS MANUALLY PLACED ON THE SERVER:
download_custom_buttons_json=/somefolder/download_custom_button_avm.json

Checks

[DONE] I have tested the docker "web-and-data" container with loose files. All 3 of these variants work successfully in that scenario:

WORKS NOW:
download_custom_buttons_json=/cbioportal-webapp/custom_buttons/download_custom_button_avm.json
download_custom_buttons_json=custom_buttons/download_custom_button_avm.json
download_custom_buttons_json=classpath:custom_buttons/download_custom_button_avm.json

[TODO] This needs to be tested in the "web" container with just the app.jar. The "classpath" syntax should work, since this pattern is used in other places, but I have been unable to test this image to confirm. I get an error

Error: Could not find or load main class org.cbioportal.PortalApplication
Caused by: java.lang.ClassNotFoundException: org.cbioportal.PortalApplication: 

[TODO] This needs to be regressed to make sure that existing instances that use these properties and are referencing loose files this way still work in the "web" container with just app.jar. For example, if there is an instance that uses something like this:

VERIFY STILL WORKS:
oncoprint.clinical_tracks.config_json=/somefolder/oncoprint-default-tracks.json

This should work based on the above tests with the "web-and-data" container, but to be safe, it should be tested with the "web" container.

Reviewers

@alisman

@pappde
Copy link
Contributor Author

pappde commented Aug 16, 2024

As noted above, need to figure out how to locally test the "app.jar" docker container to make sure that everything will work in the final deployment config, but running into issues testing that. Building it like this:

docker build -t cbioportal/cbioportal:my-dev-image -f docker/web/Dockerfile .

Denis Papp added 2 commits August 19, 2024 16:50
…tResourceAsStream() instead of ResourceUtils.getFile()

- check both the file system and the classpath
@pappde
Copy link
Contributor Author

pappde commented Aug 20, 2024

Reading from app.jar: Appears that ResourceUtils.getFile() does not work in that scenario, so updated to use ClassLoader.getResourceAsStream()

@pappde
Copy link
Contributor Author

pappde commented Aug 21, 2024

confirmed that the new version of readFile works with app.jar resource and with the file system. Added logging that adds visibility to what files are loaded by this code, and from where (file or jar).

app.jar resources (i.e. repo files) can be referenced both with relative paths, or with "classpath:{relativePath}". The "classpath:" prefix is not necessary.

Copy link
Collaborator

@haynescd haynescd left a comment

Choose a reason for hiding this comment

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

Great Find/Fix! Just small changes requested

@pappde
Copy link
Contributor Author

pappde commented Aug 21, 2024

Great Find/Fix! Just small changes requested

Added a commit that splits into an additional method "locateFile". It returns an InputStream, either from a "File" or a Resource inside the jar.

Copy link
Collaborator

@haynescd haynescd left a comment

Choose a reason for hiding this comment

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

lgtm

@haynescd haynescd merged commit ff85ba4 into cBioPortal:master Aug 22, 2024
11 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants