Skip to content

Modifiable related links protocol #521

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

Closed
joshhubers opened this issue Jun 12, 2019 · 11 comments
Closed

Modifiable related links protocol #521

joshhubers opened this issue Jun 12, 2019 · 11 comments
Assignees

Comments

@joshhubers
Copy link
Contributor

Description

A bit of an issue I've ran into is that when related links are created for related models it uses the protocol from the default configuration. This is problematic as I'm running an application behind a load balancer/proxy with SSL termination. So while the server itself is only http, the related links need to be https for consuming applications to fetch related resources over https.

I know links can be disabled, but that's not exactly a great solution for when links are needed for certain consumers and frameworks.

I'd be happy to look into this if I get time.
...

Environment

  • JsonApiDotNetCore Version: 3.1.0
  • Other Relevant Package Versions:
@maurei
Copy link
Member

maurei commented Jun 16, 2019

Sounds like there is no way to register your own LinkBuilder and provide JsonApiDotNetCore with custom linkbuilding? I feel this would be the way to fix your problem.

@maurei
Copy link
Member

maurei commented Jun 18, 2019

@joshhubers

The way I propose to solve this is the following. Right now, LinkBuilder is not provided through DI but is instantiated internally by DocumentBuilder and JsonApiContext. I propose to change so that LinkBuilder implements ILinkBuilder:

public interface ILinkBuilder
{
    string GetBasePath(HttpContext context, string entityName);
    string GetPageLink(int pageOffset, int pageSize);
    string GetRelatedRelationLink(string parent, string parentId, string child);
    string GetSelfRelationLink(string parent, string parentId, string child);
}

public class LinkBuilder : ILinkBuilder
{
    ...
}

And then register the current LinkBuilder class as the default implementation. You can then register a manual implementation that suits your needs.

Let me know if this works for you. I think I will be able to implement this in the follow week or the one after. Or if you want to give it a try feel free.

@joshhubers
Copy link
Contributor Author

That works for me!

Thanks for the thoughts! I can implement this myself in the same time period, this week or the next and open a PR.

@ngboardway
Copy link

It looks like a lot of this work is being started in #504 (I noticed the introduction of the ILinkBuilder interface). Waiting to see how that plays out before duplicating the refactoring that's happening there.

@maurei
Copy link
Member

maurei commented Jun 24, 2019

It will take at least several more weeks for #506 (which is the PR for #504) to be finished and merged. This is because the scope of the project to completely decouple IJsonApiContext is a lot bigger than just refactoring LinkBuilder into ILinkBuilder. So feel free to open a separate PR and work on ILinkBuilder so we can release an alpha to make it available earlier for you guys

@wisepotato
Copy link
Contributor

Yeah i'll be working on #506 next week, but wont finish probably

@ngboardway
Copy link

I think we're still going to wait. We have a workaround in place for the time being and like the direction the PR is heading (especially the request manager). We'll check back in a few weeks once you've had a chance to work on it more and see if/ what more still needs to be done.

@wisepotato
Copy link
Contributor

I would suggest proposing a PR that introduces an ILinkBuilder for your needs, and I will integrate this into #506 , so that no duplication will be necessary. Or you can wait, up to you :)

@wisepotato wisepotato added this to the v4.0 milestone Jun 26, 2019
@wisepotato wisepotato self-assigned this Jun 26, 2019
@ngboardway
Copy link

What we need is to be able to override the scheme of the base path. (based on your pr) this has technically been moved out of the LinkBuilder altogether. If the base path is only set in the request middleware, we're thinking that adding an option to the I/JsonApiOptions could be more helpful. Whether that's a boolean that the scheme should always use https, providing a string, etc.

@maurei
Copy link
Member

maurei commented Jun 28, 2019

My intuition says that says that base path should still be part of LinkBuilder and @wisepotato was planning on putting it back in there but didn't get there yet. I could be wrong though, I haven't thought it through in much detail

@wisepotato wisepotato removed this from the v4.0 milestone Oct 9, 2019
@maurei
Copy link
Member

maurei commented Oct 10, 2019

See #558

@maurei maurei closed this as completed Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants