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

used _baseUrl.AbsoluteUri instead of _baseUrl to avoid included port number in the self link #206

Merged

Conversation

sergey-litvinov-work
Copy link
Contributor

@sergey-litvinov-work sergey-litvinov-work commented Oct 9, 2018

Hi Jouke,

This is small PR to fix one small issue. It happens for us only when we host it under IIS (Win10\WinServer 2016 and probably all others too).

The issue is that top self link always includes port even if port is standard like this:

    "links": {
        "self": "http://localhost:80/SauleExample/api/values"
    }

or like this

    "links": {
        "self": "https://localhost:443/SauleExample/api/values"
    }

It's still valid url as it can contain port, but it looks a bit weird. Somehow IIS always pushes port in the Request.Url and _baseUrl.OriginalUrl always has it, and when we pass Uri as object for serialization, then Newtonsoft.Json uses .OriginalUrl and it gives us 80\443 ports for regular http\https hosting.

I pushed change to use _baseUri.AbsoluteUri in the ResourceSerializer and also updated a couple of unit tests as they were using Uri from JObject but now it has just regular string. I also added unit test for ResourceSerializer to test this specific case. Also i verified that non standard ports still are displayed under IIS\IIS Express

Here is also screenshot of VisualStudio watch that shows current values during debug

sauleexample_debug

Full response example before fix looks like it:

{
    "data": [
        {
            "type": "person",
            "id": "1",
            "links": {
                "self": "http://localhost/SauleExample/api/people/1/"
            },
            "attributes": {
                "name": "some person",
                "age": 42
            },
            "relationships": {
                "job": {
                    "links": {
                        "self": "http://localhost/SauleExample/api/people/1/relationships/job/",
                        "related": "http://localhost/SauleExample/api/people/1/job/"
                    },
                    "data": {
                        "type": "company",
                        "id": "11"
                    }
                }
            }
        }
    ],
    "links": {
        "self": "http://localhost:80/SauleExample/api/values"
    },
    "included": [
        {
            "type": "company",
            "id": "11",
            "links": {
                "self": "http://localhost/SauleExample/api/companies/11/"
            },
            "attributes": {
                "name": "Some company",
                "location": "some location"
            }
        }
    ]
}

@joukevandermaas joukevandermaas self-requested a review October 10, 2018 05:43
@sergey-litvinov-work
Copy link
Contributor Author

@joukevandermaas just wanted to double check, you want me to do something more there or you just waiting for something? as you haven't merged these changes :)

@joukevandermaas
Copy link
Owner

@sergey-litvinov-work my apologies, I got distracted by other things and completely forgot.

@joukevandermaas joukevandermaas merged commit 0ddeaf4 into joukevandermaas:master Oct 12, 2018
@sergey-litvinov-work
Copy link
Contributor Author

Thanks!

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.

2 participants