Repository: ithaka/apiron

Issue:
Non-intuitive behavior with non-root endpoints
**Describe the bug**
Non-root endpoints - e.g., example.com/rest - behave non-intuitively when paired with their endpoints.

**To Reproduce**
Steps to reproduce the behavior:
1. Define: domain = 'https://example.com/server'
1. Add some endpoints: test = JsonEndpoint('/test')
1. Call endpoint: client queries url https://example.com/test

**Expected behavior**
The intuitive behavior is joining the full domain with the endpoint. That is, 'https://example.com/server/test'

**What actually happened**
The culprit is urllib.parse.urljoin. It is quite fastidious about joining urls. The 'workaround' is that you must define the domain with a trailing slash and the endpoint missing its leading slash.

**Additional context**
There are other combos that create non-intuitive joins. On the one hand, urllib.parse.urljoin is most certainly correct in how it goes about joining. On the other hand, as many rest endpoints are not placed on the root of the server, I do believe some clarification is needed. Domain and endpoint definition probably shouldn't require understanding urljoin specifics. There are no doubt a few choices here. Easiest is likely more documentation, which I can help add if that's the route you choose. I believe forcing domains to trailing slashes and removing leading slashes from endpoints would work, but I'm not sure about full consequences.

Anyways, thanks for great library!


Discussion:
daneah: Defaulting to the more intuitive joining behavior will break the following three cases, which like you've said in #13 deliver a surprise result of `'http://biz.com/endpoint'` despite being _technically_ correct (per [IETF RFC 1808](https://tools.ietf.org/html/rfc1808.html), upon further reading):

```
host = 'http://biz.com/v2', path = '/endpoint', url = 'http://biz.com/v2/endpoint'
host = 'http://biz.com/v2/', path = '/endpoint', url = 'http://biz.com/v2/endpoint'
host = 'http://biz.com/v2', path = 'endpoint', url = 'http://biz.com/v2/endpoint'
```

I'm inclined to agree that we go the route of intuition and explicitly state nonconformity to `urljoin`'s relative endpoint swapping. I don't think we're relying on this behavior internally, and users who'd like to have endpoints that are back at the root of the domain or down a different subpath can explicitly structure their `Service`s that way, like you said. This would allow for pulling the building of the URL up into `ServiceCaller` which significantly simplifies the testing. I've got that approach on a branch locally that I could push to your fork, if you don't mind! I'm not sure how to tell if you've chosen the "Allow edits from maintainers" option...

Note that this would indeed be a breaking change (though hopefully not one people are using widely), so will need to go into a `2.0.0` release.
teffalump: Thanks for the thoughtful response. You have stated it much better than I did. As you said, defining the REST structure is more intuitive if we tip-toe, carefully, around those RFC standards. I suppose if the goal is to be opinionated the `smart_urljoin` option is unnecessary - simply state that separate roots require defining separate services or similar.

That is true about breaking change - I did not consider that angle. The "Allow edits from maintainers" option is checked - push away! :+1:
daneah: @teffalump I pushed my change; please let me know what you think. I'll have some other folks review on Monday as well.
daneah: @momo-sa it accounts for any time path does not contain a leading slash; a little flavor of of the [robustness principle](https://en.wikipedia.org/wiki/Robustness_principle) 😄 
teffalump: Looks great! Thanks for responses. 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d45d842..6721b0b 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+### Added
+- The `check_for_trailing_slash` behavior can now be used on `JsonEndpoint`s
+
+### Changed
+- Service domains and endpoint paths were previously joined per [IETF RCF 1808](https://tools.ietf.org/html/rfc1808.html),
+  which has surprising edge cases.
+  These are now always joined by appending the endpoint path;
+  the previous behavior can be achieved with multiple services or endpoints as needed.
+
 ## [1.1.0] - 2018-12-03
 ### Added
 - Exposed ability to explicitly set the response encoding
diff --git a/apiron/client.py b/apiron/client.py
index 8417517..e278335 100644
--- a/apiron/client.py
+++ b/apiron/client.py
@@ -35,6 +35,27 @@ class ServiceCaller:
     A class for calling :mod:`services <apiron.service.base>`
     """
 
+    @staticmethod
+    def build_url(host, path):
+        """
+        Builds a valid URL from a host and path which may or may not have slashes in the proper place.
+        Does not conform to `IETF RFC 1808 <https://tools.ietf.org/html/rfc1808.html>`_ but instead joins the host and path as given.
+        Does not append any additional slashes to the final URL; just joins the host and path properly.
+
+        :param str host:
+            An HTTP host like ``'https://awesome-api.com/v2'``
+        :param str path:
+            The path to an endpoint on the host like ``'/some-resource/'``
+        :return:
+            The properly-joined URL of host and path, e.g. ``'https://awesome-api.com/v2/some-resource/'``
+        :rtype:
+            str
+        """
+        host += '/' if not host.endswith('/') else ''
+        path = path.lstrip('/')
+
+        return parse.urljoin(host, path)
+
     @staticmethod
     def get_adapted_session(adapter):
         """
@@ -100,7 +121,7 @@ class ServiceCaller:
 
         request = requests.Request(
             method=method or endpoint.default_method,
-            url=parse.urljoin(host, path),
+            url=cls.build_url(host, path),
             params=merged_params,
             data=data,
             headers=headers,


Test patch:
diff --git a/tests/test_client.py b/tests/test_client.py
index 1cdd15c..caf3be6 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -1,6 +1,8 @@
 import unittest
 from unittest import mock
 
+import pytest
+
 from apiron.client import ServiceCaller
 from apiron.exceptions import NoHostsAvailableException
 
@@ -234,3 +236,16 @@ class ClientTestCase(unittest.TestCase):
         service.get_hosts.return_value = []
         with self.assertRaises(NoHostsAvailableException):
             ServiceCaller.choose_host(service)
+
+@pytest.mark.parametrize('host,path,url', [
+    ('http://biz.com', '/endpoint', 'http://biz.com/endpoint'),
+    ('http://biz.com/', 'endpoint', 'http://biz.com/endpoint'),
+    ('http://biz.com/', '/endpoint', 'http://biz.com/endpoint'),
+    ('http://biz.com', 'endpoint', 'http://biz.com/endpoint'),
+    ('http://biz.com/v2', '/endpoint', 'http://biz.com/v2/endpoint'),
+    ('http://biz.com/v2/', 'endpoint', 'http://biz.com/v2/endpoint'),
+    ('http://biz.com/v2/', '/endpoint', 'http://biz.com/v2/endpoint'),
+    ('http://biz.com/v2', 'endpoint', 'http://biz.com/v2/endpoint'),
+])
+def test_build_url(host, path, url):
+    assert url == ServiceCaller.build_url(host, path)