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.

Sign up for GitHub

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

Jump to bottom

Generated selfs links on top level does not respect ServiceUrlProvider #789

Closed
choeflake opened this issue Oct 13, 2020 · 6 comments · Fixed by #815 · May be fixed by #793
Closed

Generated selfs links on top level does not respect ServiceUrlProvider #789

choeflake opened this issue Oct 13, 2020 · 6 comments · Fixed by #815 · May be fixed by #793
Labels
help wanted

Comments

@choeflake
Copy link

The self link which is generated in DocumentMapper (line 146-148) does not take the ServiceUrlProvider in advance. So the resulting link uses the host and path from the request URI. Which could lead to wrong links when the system is behind a proxy / load balancer etc.
@pekka-ia pekka-ia mentioned this issue Oct 31, 2020
Open
@turtleman
Copy link

turtleman commented Nov 2, 2020 •
edited
Loading

Thanks for @pekka-ia providing a solution for this issue! Here's hoping the proposed solution fits to the project's goals.

@choeflake
Copy link
Author

Thanks @pekka-ia looks good! Hopefully the request will be approved soon!

@pekka-ia
Copy link

pekka-ia commented Nov 2, 2020

I realized there is actually a tricky thing with ServiceUrlProvider and the reactive projects - basically ServiceUrlProvider is attempting to access HttpRequestContext through thread context (HttpRequestContextProvider#33-38) which is not available in reactive settings.

I'll see if I can make this be compatible with both contexts.

@pekka-ia
Copy link

pekka-ia commented Nov 2, 2020 •
edited
Loading

Looks like on the resource links side this base URL building is done like so:

https://github.com/crnk-project/crnk-framework/blob/master/crnk-core/src/main/java/io/crnk/core/engine/internal/registry/ResourceRegistryImpl.java#L117-L120

I'll see if I can introduce similar logic here.

@pekka-ia
Copy link

pekka-ia commented Nov 2, 2020 •
edited
Loading

I think I have a solid fix now. Instead of attempting to use ServiceUrlProvider, using requestContext.getBaseUrl() seems to yield exactly the same results.

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"
    }
}

@turtleman
Copy link

@pekka-ia, excellent!

@vicmosin vicmosin mentioned this issue May 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
4 participants
@turtleman @choeflake @remmeier @pekka-ia