Skip to content

Fixed: RelativeLinks #766

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

Merged
merged 4 commits into from
May 22, 2020
Merged

Conversation

fdlane
Copy link
Contributor

@fdlane fdlane commented May 20, 2020

Closes #763

Added commented out options.RelativeLinks = true; to the GettingStarted example project.

@bart-degreed
Copy link
Contributor

Added commented out options.RelativeLinks = true; to the GettingStarted example project.

Why?

The solution (adding a / to the base path) does not seem to have anything to do with absolute/relative links. I'd expect the / to be already in the base path if needed.

We should have an integration test for relative links. you can do:

var options = (JsonApiOptions)_factory.GetService<IJsonApiOptions>();
options.UseRelativeLinks = true;

from within a test, before executing a request. Another approach would be to do something similar to NoDefaultPageSizeStartup.

@fdlane fdlane force-pushed the fix-relative-links branch from c394e1f to 3e7b658 Compare May 21, 2020 14:37
@fdlane
Copy link
Contributor Author

fdlane commented May 21, 2020

@bart-degreed adding the option in the GettingStarted was an attempt to help new users. I reverted the change.

Tests have been added for true/false.

The change I made seems to work with the responses in the Tests, GettingStarted app, and my apps tested locally.

I reviewed the links being created in the following blocks and it seems to behave as expected now.

return $"{GetBasePath()}/{parent}/{parentId}/relationships/{navigation}";

return $"{GetBasePath()}/{resource}/{resourceId}";

~

return $"{GetBasePath()}/{parent}/{parentId}/{navigation}";

@bart-degreed
Copy link
Contributor

I reviewed the links being created in the following blocks and it seems to behave as expected now.

That the code works does not mean it is the right solution. The base path should be set in middleware and be correct in all cases. I've added tests and fixed the problem to show what I had in mind. Let me know if you agree with this.

I also tried to cleanup the GetCustomRoute method, but found no good way up till now. Not needed for this PR, though.

@fdlane
Copy link
Contributor Author

fdlane commented May 22, 2020

@bart-degreed, so much better. Thank you.

@bart-degreed bart-degreed merged commit fdebb81 into json-api-dotnet:master May 22, 2020
@bart-degreed
Copy link
Contributor

@fdlane Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Namespace not honored when RelativeLinks=true
2 participants