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(RDF API): Incorrect internal IRIs #4383

Merged
merged 16 commits into from
Oct 28, 2024
Merged

Conversation

svandenhoek
Copy link
Contributor

@svandenhoek svandenhoek commented Oct 21, 2024

What are the main changes you did:

how to test:

  • See if behaviour of RDF API output generates incorrect internal IRIs #4382 does not occur anymore
  • Check if URL still correct regarding presence/absence port:
    • On local server: curl http://localhost:8080/pet%20store/api/rdf should contain <http://localhost:8080/pet%20store/api/rdf/User/column/username>
    • On preview server: curl https://preview-emx2-pr-4383.dev.molgenis.org/pet%20store/api/rdf should contain <http://preview-emx2-pr-4383.dev.molgenis.org/pet%20store/api/rdf/User/column/username>

todo:

  • updated docs in case of new feature
  • added/updated tests
  • added/updated testplan to include a test for this fix, including ref to bug using # notation

@svandenhoek svandenhoek marked this pull request as ready for review October 21, 2024 15:19
Copy link
Member

@harmbrugge harmbrugge left a comment

Choose a reason for hiding this comment

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

I think using contextPath wil fix the issue, but indeed a test would be nice

Copy link
Member

@esthervanenckevort esthervanenckevort left a comment

Choose a reason for hiding this comment

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

  • In general the code looks fine
  • As already mentioned it would be good to add a test for the change in extractBaseUrl
  • Update with main first

import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

I thought it is not preferable to use * imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDEA did that automatically after I made some changes, I'll make sure to revert it.

@svandenhoek
Copy link
Contributor Author

Seems config.router.contextPath (as stated in the Javalin docs) is currently not being configured in:

Javalin app =
Javalin.create(
config -> {
config.http.maxRequestSize = MAX_REQUEST_SIZE;
config.router.ignoreTrailingSlashes = true;
config.router.treatMultipleSlashesAsSingleSlash = true;
config.jetty.modifyServletContextHandler(
handler -> handler.setSessionHandler(sessionManager.getSessionHandler()));
})
.start(port);

So the current fix might not work yet anyhow (or any other places that depend on a defined contextPath)...

@svandenhoek
Copy link
Contributor Author

Tests were added where config.contextResolver.host & config.router.contextPath are set to test the behaviour. In my initial feedback processing (510b1b5) Javalin actually used the said port (and locally all tests worked), but the Azure build failed due to java.net.BindException: Permission denied on port 80.

So currently using the Javalin test framework that uses a random uncommon port and simply overriding the defined host. This means some of the tests were removed though that used the actual port.

Copy link

sonarcloud bot commented Oct 28, 2024

@svandenhoek svandenhoek merged commit 90565b5 into master Oct 28, 2024
6 checks passed
@svandenhoek svandenhoek deleted the fix/rdf_api_base_url branch October 28, 2024 13:29
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.

RDF API output generates incorrect internal IRIs
3 participants