-
-
Notifications
You must be signed in to change notification settings - Fork 493
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
datahubs per sub-portal #8644
base: main
Are you sure you want to change the base?
datahubs per sub-portal #8644
Conversation
2fbf024
to
30d9db8
Compare
Awesome !! 🤩 |
4b09816
to
a351411
Compare
a351411
to
dd54831
Compare
@Guillaume-d-o could you check the CI please, seems there is an issue during the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Haven't tested yet but I've seen it work. We should address my comments first and then I'll do another final review.
web/src/main/webapp/WEB-INF/classes/setup/sql/migrate/v447/migrate-default.sql
Outdated
Show resolved
Hide resolved
## Geonetwork-ui configuration | ||
|
||
*Enable datahub*: | ||
You can adapt your GeoNetwork UI by enabling Datahub. First, download the specific plugin datahub-integration. Then, on the settings page, enable Datahub . you can provide an other specific Datahub configuration than the default one. You can also configure it for each portal. Ensure that Datahub is enabled in the settings to make it work on portals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for documenting your work. IMO this should go in a separate section of the documentation. The things that need to be documented are:
- what the plugin does (with a link to GeoNetwork-UI project)
- how to build the plugin
- how to enable it
- how to configure the datahub instances (link to the relevant documentation in GN-UI)
We might start a new "plugins" section in the documentation for that, I think that'd be appropriate. @jodygarnett do you have any opinion on how/where to put this? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! It's looking good, I made several suggestions to the code though, could you please look into them? As for the documentation I'll add a commit to your branch to make it clearer if that's OK.
@Autowired | ||
SettingManager settingManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
handleDatahubRequest(request, response, locale); | ||
} | ||
|
||
void handleDatahubRequest(HttpServletRequest request, HttpServletResponse response,String locale) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many lines have inconsistent code formatting; could you run a code formatting task with your IDE?
} | ||
|
||
void handleDatahubRequest(HttpServletRequest request, HttpServletResponse response,String locale) throws IOException { | ||
Log.debug(LOGGER_NAME, "enter in datahub"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary
int pathPartToSkip = 3;// "/srv/datahub/bla/bla" | ||
if (locale != null) { | ||
pathPartToSkip = 4;// /srv/fre/datahub/bla/bla | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little bit hacky; ideally the isolation of the path in the request should have been done earlier; to keep in mind if we have to rework this
|
||
String filePath = Stream.of(reqPath.split("/")).skip(pathPartToSkip).collect(Collectors.joining("/")); | ||
if (!FileUtils.fileExistsInJar("/datahub/" + filePath)) { | ||
return new File(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here was to cause an explicit exception of type "File ... not found" that can bubble up to the log I think? if yes, we should leave a comment because otherwise this line makes no sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need it during the test phase but i can throw an exception directly
return FileUtils.getFileFromJar("/datahub/" + filePath); | ||
} catch (IOException e) { | ||
Log.error(LOGGER_NAME, e.getMessage()); | ||
return new File(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, see above, leave a comment to explain
} | ||
String configuration = ""; | ||
if (portal != null && !portal.getDatahubConfiguration().isEmpty()) { | ||
configuration = portal.getDatahubConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we also need to implement a fallback; if the configuration of the subportal is empty, we should use the one from the main portal.
id="datahubEnabled" | ||
data-ng-model="source.datahubEnabled" | ||
/> | ||
<p class="help-block" data-translate="">sourceDatahubEnabled-help</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the translation keys need to be added to the translation file here: https://github.com/geonetwork/core-geonetwork/blob/main/web-ui/src/main/resources/catalog/locales/en-admin.json
@@ -3,3 +3,6 @@ UPDATE Settings SET value='SNAPSHOT' WHERE name='system/platform/subVersion'; | |||
|
|||
INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('system/banner/enable', 'false', 2, 1920, 'n'); | |||
INSERT INTO Settings (name, value, datatype, position, internal) VALUES ('system/auditable/enable', 'false', 2, 12010, 'n'); | |||
|
|||
ALTER TABLE Sources ADD COLUMN datahubEnabled BOOLEAN DEFAULT FALSE; | |||
ALTER TABLE Sources ADD COLUMN datahubConfiguration TEXT DEFAULT ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A default configuration should be provided for this column since with an empty configuration the datahub app will crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done below :)
} | ||
|
||
File actualFile = getRequestedFile(request); | ||
if (!actualFile.exists()) { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
response.setStatus(HttpServletResponse.SC_OK); | ||
String extension = actualFile.getName().toLowerCase(); | ||
String contentType = extension.equals("js") ? "text/javascript; charset=UTF-8" | ||
: Files.probeContentType(actualFile.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
void writeResponseContent(HttpServletRequest request, HttpServletResponse response, File actualFile, | ||
String portalName) throws IOException { | ||
InputStream inStream = actualFile.getName().equals("default.toml") ? readConfiguration(portalName) | ||
: new FileInputStream(actualFile); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
This is the continuation of the proof-of-concept for the proposal #8021
This work aim to create and manage a datahub in a plugin. The Datahub application, once compiled, is a collection of static files (HTML, JS, CSS) that can be served as is. If you want more information, I strongly encourage you to read the proposal #8021 that explain all the developement in details.
We tried to maximise all the code on the plugin side.
Checklist
main
branch, backports managed with labelREADME.md
filespom.xml
dependency management. Update build documentation with intended library use and library tutorials or documentation