Architecture of Endpoints is not RESTful

[tinakurian]

Issue Overview

The architecture for launcher-backend endpoints is incorrect.

Consider the following APIs:

/services/openshift/clusters
/services/openshift/clusters/all 

It is unclear looking at these two endpoints what the difference is. In true REST fashion, these endpoints should have been something more along the lines of the following:

/services/openshift/current-user/cluters
/services/openshift/clusers

The reason why /services/openshift/clusters/all would be an incorrect endpoint is because "all" is not a representation of a resource.

5.2.1.1 Resources and Resource Identifiers The key abstraction of information in REST is a resource. Any information that can be named can be a resource: a document or image, a temporal service (e.g. “today’s weather in Los Angeles”), a collection of other resources, a non-virtual object (e.g. a person), and so on. In other words, any concept that might be the target of an author’s hypertext reference must fit within the definition of a resource. A resource is a conceptual mapping to a set of entities, not the entity that corresponds to the mapping at any particular point in time.

Taken from: Architectural Styles and the Design of Network-based Software Architectures Dissertation by Roy Thomas Fielding: https://www.ics.uci.edu/~fielding/pubs/dissertation/fielding_dissertation.pdf

Furthermore, it is confusing to have the same endpoints repeated several times under different headings on the swagger spec. Consider the following:

Openshift.io
GET
/services/jenkins/pipelines
Jenkins

Jenkins
GET
/services/jenkins/pipelines

Services
GET
/services/jenkins/pipelines

Expected Behaviour

/services/openshift/current-user/cluters
/services/openshift/clusers

OR

/services/openshift/{current-user-id}/cluters
/services/openshift/clusers

Openshift.io
GET
/services/jenkins/pipelines

Current Behaviour

/services/openshift/clusters
/services/openshift/clusters/all 

Openshift.io
GET
/services/jenkins/pipelines
Jenkins

Jenkins
GET
/services/jenkins/pipelines

Services
GET
/services/jenkins/pipelines

Steps To Reproduce

https://editor.swagger.io/?url=https://forge.api.openshift.io/swagger.yaml

[aslakknutsen]

In true REST fashion, the URL path structure is completely irrelevant.

[stevengutz]

@aslakknutsen I don't think I agree with the comment above. REST design is very specific on the URL structure. The URL is supposed to a path to a resource, so it is hardly irrelevant. In the example above "/services/openshift/clusters/all" isn't a resource - it's a function call in disguise. Since we are supposed to be a company catering to software engineers we should probably build API endpoints consistent with the rest of the industry.

I agree with @tinakurian here - and I know she has a bit of history in REST endpoint design. I think we should revisit our endpoints, at least for launcher, to clean them up.

[bartoszmajsak]

@stevengutz Can you define "the rest of the industry"? Because for example, this very thing I'm putting my comment in is moving away from REST to GraphQL for a lot of good reasons (including scalability, easiness and flexibility for 3rd party integrators).

Maybe we should take one step back and think if REST and resources is really a way to go for this set of services?

As for the issue itself - I agree with the need of making the things right and where we can we should pay back the debt we have, but let's not make architectural decisions on a particular little service used for very little chunk of the whole thing we are building here.

[aslakknutsen]

@stevengutz Why isn't /x/all a resource? What determines that it would be, if it was just /xs/? Does a program calling /x/all somehow behave differently because this URL has a path named as a 'function call in disguise', how?

Or perhaps there are completely different aspects of the 'URL handlers' behavior that determines it's 'restfullness'...

[bartoszmajsak]

Furthermore, it is confusing to have the same endpoints repeated several times under different headings on the swagger spec.

@tinakurian I think the confusion here is the swagger "tag". The idea, if I understand that correct, was to show you which services belong to which category. Maybe Services tag is one step too far, but Openshift.io hints here that this is used by OSIO, whereas Jenkins tag means that this particular service exposes "jenkins stuff".

[stevengutz]

@bartoszmajsak You're right and I'm all for going to GraphQL, let's do it!

@aslakknutsen. if we were REST purists (and I probably am), we would have "/services/openshift/cluster" to return the current cluster and "/services/openshift/clusters" to return all clusters. Lots of people will make an argument that as long as you don't have something like "/services/openshift/cluster/get" then you're RESTful - everyone can have an opinion, mine is maybe a bit more idealistic because I don't think you should have to read a bunch of docs to understand the API. I like clean, concise endpoints, so shoot be for being a purist. ElasticSearch for example is the worst REST endpoints ever put to code, but it's successful regardless - doesn't make it right though.

[alexeykazakov]

I don't think you should have to read a bunch of docs to understand the API

I don't think it's possible in any real world complex app but we should try our best to go to that direction for sure :)

But I really don't care much if it's pure canonical REST or something else. So, I would work on the legit issue "Our API is confusing. Let's improve it!" rather than opening debuts about what is pure and true REST and what is not.

[aslakknutsen]

@stevengutz If you are a REST purist you would know the main purpose of REST is to decouple client and server, so why are you hardcoding knowledge of multiple endpoints in the client? You expose a root(api.os.io) and let the client traverse the logical links exposed resource by resource, at which point the URL path becomes completely irrelevant. At no point does the client use the URL structure/name/path to gain any knowledge about the target. It's a pointer, that's all.

[tinakurian]

I agree with @alexeykazakov I agree. The whole point is that it's confusing.

@stevengutz I agree. I've seen many confusing things defended. I have even seen a POST endpoint performing a delete action in a database... it was very very confusing.

@aslakknutsen The root api idea which you proposed for planner (which was implemented last sprint) was a brilliant idea! We should definitely do that in launcher as well. Definitely helps further decoupling the client and server. Part of the purpose of using tools like swagger is to generate documentation to help the endpoints become more easily understandable by people. Creating endpoints which are easy to understand and using conventions that people are used to seeing will help in making a product that the public + our evolving team can grasp a little easier. And hey, if we do that, maybe we could get more people helping out on our opensource project - which would be a pretty cool side effect.

@bartoszmajsak I agree that the service tag was a little much :-) GraphQL will be cool, maybe this is something we can look into at some point?