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

Modified DocumentMapper to use ServiceUrlProvider for generating document self links #793

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pekka-ia
Copy link

This allows for relative urls (by emptying crnk.domain-name) and also improves configurability in situations where the system runs behind a proxy.

Fixes #789

…ment self links. This allows for relative urls (by emptying crnk.domain-name) and also improves configurability in situations where the system runs behind a proxy.
@pekka-ia
Copy link
Author

pekka-ia commented Nov 2, 2020

Taking another look at this, there was something odd with the behaviour in tests vs. behaviour in use.

Pekka Iso-Ahola added 2 commits November 2, 2020 12:02
@pekka-ia
Copy link
Author

pekka-ia commented Nov 2, 2020

I think I have a solid fix now. Instead of attempting to use ServiceUrlProvider, using requestContext.getBaseUrl() seems to yield exactly the same results and it doesn't break functionality within reactive context.

When the below config is applied:

crnk.domain-name: ""

then

GET http://localhost:21120/api/incentive-programs?fields[incentive-programs]=name

{
    "data": [
        {
            "links": {
                "self": "/api/incentive-programs/8377ecb2-d516-4ee1-9fd3-259c54a9119f"
            }
            // rest of the object
        }
    ],
    "links": {
        "self": "/api/incentive-programs?fields[incentive-programs]=name"
    }
}

With an explicit domain set, it looks like this:

crnk.domain-name: "https://www.nbc.com"

GET http://localhost:21120/api/incentive-programs?fields[incentive-programs]=name

{
    "data": [
        {
            "links": {
                "self": "https://www.nbc.com/api/incentive-programs/a2081411-5961-4841-8570-6d538ad51f11"
            }
            // rest of the object
        }
    ],
    "links": {
        "self": "https://www.nbc.com/api/incentive-programs?fields[incentive-programs]=name"
    }
}

and with no domain-name configuration set at all, it still works as expected

GET http://localhost:21120/api/incentive-programs?fields[incentive-programs]=name
{
    "data": [
        {
            "links": {
                "self": "http://localhost:21120/api/incentive-programs/bf871ce6-8f25-45c9-ae69-71823bbef46b"
            }
            // rest of the object
        }
    ],
    "links": {
        "self": "http://localhost:21120/api/incentive-programs?fields[incentive-programs]=name"
    }
}

@remmeier
Copy link
Contributor

It is a bit a dangerous area for changes because of the variety of integrations (servlet, etc.). Where have you seen that reqestUri != baseUrl + path? Because that would be wrong and in need for fixing. There might be other places where reqestUri is used and also breaks.

@vicmosin
Copy link
Contributor

@remmeier Can we please merge this change? I need self link to be generated based on ServiceUrlProvider as well

@@ -143,7 +145,8 @@ private LinksInformation enrichSelfLink(LinksInformation linksInformation, Query
linksInformation = selfLinksInformation;
}

JsonApiUrlBuilder.UrlParameterBuilder urlBuilder = new JsonApiUrlBuilder.UrlParameterBuilder(requestUri.toString());
JsonApiUrlBuilder.UrlParameterBuilder urlBuilder = new JsonApiUrlBuilder.UrlParameterBuilder(UrlUtils.concat(queryAdapter.getQueryContext().getBaseUrl(),
Copy link
Contributor

Choose a reason for hiding this comment

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

what isnwring with the request uri? maybe a fix should be applied there if necessary. currently the code is inconsitent, having null check for resourceuri, but does not make use it.

@vicmosin
Copy link
Contributor

@pekka-ia are you gonna provide the changes @remmeier asked for?

@pekka-ia
Copy link
Author

@pekka-ia are you gonna provide the changes @remmeier asked for?

I don't think I can currently do it. I don't have a good environment for doing Java development currently (this PR was opened while I was working for my former employer). :/

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.

Generated selfs links on top level does not respect ServiceUrlProvider
3 participants