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

Base path config option is not included in REST API responses when using non-default domains #990

Closed
sgohl opened this issue Jan 27, 2021 · 10 comments · Fixed by #998
Closed
Labels
Milestone

Comments

@sgohl
Copy link

sgohl commented Jan 27, 2021

I recently opened a feature request #989

The answer based on the assumption of serving the app on the BASE_PATH, which isn't actually the problem really.

I have NOT set SHORT_DOMAIN_HOST anymore on purpose (which didn't make a difference anyway).

I need multiple domains and a fixed suffix prepended before the short_code on generating the shortcode
This is not related to serving the server-side app in a sub-URI-path.

example:

curl 'https://shlink.example.com/s/rest/v2/short-urls' ... --data-raw '{"longUrl":"https://my-long.com/URL","domain":"myprimeportal.com","findIfExists":true,"validateUrl":false}'

{"shortCode":"mKM9r","shortUrl":"https://myprimeportal.com/mKM9r" ...

the actual generated short-URL is

https://myprimeportal.com/mKM9r

instead of what I want:

https://myprimeportal.com/s/mKM9r

Error:

curl 'https://shlink.example.com/s/rest/v2/short-urls' -H 'Content-Type: application/json;charset=utf-8' -H "X-Api-Key: $KEY" --data-raw '{"longUrl":"https://myprimeportal.com/Long/URL/with?parameters=true","tags":["something"],"domain":"myprimeportal.com/s","findIfExists":true,"validateUrl":false}'

{"invalidElements":["domain"],"title":"Invalid data","type":"INVALID_ARGUMENT","status":400,"detail":"Provided data is not valid"}~ > 

It does not accept /s within domain key. But giving the default domain exactly this way with appended /s , it works.

if I edit mysql table short_urls and prepend s/ to field short_code, the app will list the wanted short-URL in the web-dashboard

This (appended BASE_PATH) only works with single domain, but not if you send a custom domain in the payload, then the s/ is missing from the generated shortURL response.

any ideas ?

(using the docker shlink:stable behind traefik:v1 with PathPrefix:/s/ and BASE_PATH=/s)

EDIT: BASE_PATH should be appended to all domains, not only the default domain; OR; make possible to payload a domain with URI path

@sgohl sgohl added the bug label Jan 27, 2021
@acelaya
Copy link
Member

acelaya commented Jan 28, 2021

BASE_PATH should work for all domains. If it's not, it's probably a bug. I'll investigate this.

I'm thinking that maybe the bug is just on composing the URL after creating it.

You mention that, when providing the domain, the response says the short URL is something in the lines of https://myprimeportal.com/mKM9r. Can you try if then, visiting https://myprimeportal.com/s/mKM9r works?

@sgohl
Copy link
Author

sgohl commented Jan 28, 2021

ha, funny, you nailed it. Although /s/ is missing in the Short URL in the webapp listing, the server responses with the correct redirect location when requested with prepended /s/

@acelaya
Copy link
Member

acelaya commented Jan 28, 2021

Thanks for confirming. It's a smaller issue then.

@acelaya acelaya added this to the 2.6.0 milestone Jan 28, 2021
@sgohl
Copy link
Author

sgohl commented Jan 28, 2021

nice, i really appreciate your investigation! if the listing would also include the BASE_PATH for every domain, I'd be very happy

@acelaya acelaya changed the title optional URI suffix prepend to short URL Base path config option is not included in REST API responses when using non-default domains Jan 28, 2021
@acelaya
Copy link
Member

acelaya commented Jan 28, 2021

Yep, definitely, that needs to be fixed. I just wanted to confirm exactly what was the issue.

@acelaya
Copy link
Member

acelaya commented Jan 31, 2021

Hey @sgohl. I have been testing this and I can't manage to reproduce it. For me it is working as expected.

If I provide the BASE_PATH to the docker image, the generated short URL in the response includes it.

I have followed these steps:

  • Spin-up the docker image, including the base path:

    docker run --name my_shlink -p 8081:8080 -e SHORT_DOMAIN_HOST=localhost:8081 -e SHORT_DOMAIN_SCHEMA=http -e BASE_PATH=\/s shlinkio/shlink:stable
  • On a different terminal, generate an API key over the running container:

    docker exec -it my_shlink shlink api-key:generate
  • Copy the generated API key, then run this command to generate a short URL (notice you have to replace [API_KEY] with the value returned in previous step):

    curl -X POST "http://localhost:8081/s/rest/v2/short-urls" -H  "accept: application/json" -H  "X-Api-Key: [API_KEY]" -H  "Content-Type: application/json" -d "{\"longUrl\":\"https://github.com/shlinkio/shlink/issues/990\"}"

After this, the response includes the shortUrl property with the /s path, as expected.

image

Could it be that you are doing something differently?

Maybe you are not properly providing the env var to the shlink container, but just to traefik, which in turn properly redirects the request to Shlink but stripping-off the path?

@acelaya
Copy link
Member

acelaya commented Feb 1, 2021

I'm stupid. The issue was reproducible only when using custom domains. Ignore my previous comment 😅

@acelaya
Copy link
Member

acelaya commented Feb 1, 2021

I reproduced it and fixed it. It will work as expected in v2.6.0

@sgohl
Copy link
Author

sgohl commented Feb 3, 2021

thanks so much for your efforts, really appreciate!

I'll test and verify as soon as I find a 2.6.0 release/docker image

@acelaya
Copy link
Member

acelaya commented Feb 14, 2021

Hey @sgohl, I released v2.6.0 yesterday, which includes the fix for this.

Give it a try and let me know if there's anything not working as expected.

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

Successfully merging a pull request may close this issue.

2 participants