search

SmartBear / git-en-boite

Web service facade for interacting with git repos on various providers - GitHub/BitBucket/GitLab etc.
MIT License
18 stars 7 forks source link

Use proper 2xx HTTP codes on connect #334

Closed jbpros closed 3 years ago

jbpros commented 4 years ago

We currently use a redirect (30x) HTTP status code on a connect request for a repo that was already connected. I believe this is an odd use of a redirect status. The 3xx (Redirection) class of status code indicates that further action needs to be taken by the user agent in order to fulfill the request. In the case of a connect request on git-en-boîte for an already connected repo, the client is not expected to take any further action to connect the repo.

I'm unclear as to why we need to tell the client about the internal differences: does it matter if the repo was already connected or not?

If it does, then I suggest we use 201 Created as the status for a connect that succeeded and actually connected a new repo, 200 OK for a successful connect for an existing repo.

If it does not matter, then we just return 200 OK on all successful connects (this has my preference for now, but I might be missing a good reason for the differentiation).

We'll need to adapt Git::GitEnBoite in CucumberStudio when we make this change.

Checklist:

mattwynne commented 4 years ago

Agreed. I think we should also return a location header with the location of the newly created resource.

Arguably, if we already know the repoId we should be POSTing directly to /repos/:repoId to create it.

mattwynne commented 4 years ago

IMO on the 201/200 I'm not sure I see a reason yet why a client would need it, but on the other hand it's good to be communicative if we have that information to hand.

jbpros commented 4 years ago

Location is to be used with 3xx and 201 and is highly focussed on automatic redirection. Link headers are a good fit for telling the client about related resources (like the related repo, or its branches or whatever relevant stuff). It is typically used in HATEOAS systems. Hypermedia has many great advantages, especially for public facing APIs like Git-en-Boîte (one of them is virtually no need for API versioning).

jbpros commented 4 years ago

I agree about the post request to a specific endpoint for the known repo ID.

mattwynne commented 3 years ago

💯 on the HATEOS thing. Let's do that before we add any more end-points.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions 🙏.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you for your contributions 🙏.

mattwynne commented 3 years ago

bump

mattwynne commented 3 years ago

We're working on this code now as part of https://github.com/hiptest/cucumberstudio/issues/1434 I suggest we take the opportunity to clean it up a little.

I would starty by changing this line to send a PUT request to /repos/{repoId}:

https://github.com/SmartBear/git-en-boite/blob/ec0ffa956ec017aa56fc452bd0298f5cb77b8817/packages/acceptance-tests/src/step_definitions/steps.ts#L79

That can then drive a change to this unit test for the router/controller:

https://github.com/SmartBear/git-en-boite/blob/main/packages/web/src/routes/repos/update.spec.ts#L30

It should be changed to behave more like this endpoint, so that it creates the repo. It should also be changed to update the repo if it already exists, with the new repoUrl.

https://github.com/SmartBear/git-en-boite/blob/main/packages/web/src/routes/repos/create.spec.ts#L41

We can be confident about changing the /repos/{repoId} endpoint - the only production client we have is CucumberStudio at the moment, which uses the POST /repos endpoint so as long as we leave that alone we won't break anything.

jbpros commented 3 years ago

It should also be changed to update the repo if it already exists, with the new repoId.

Do you mean the new repoUrl?

mattwynne commented 3 years ago

Do you mean the new repoUrl?

Yes! I've updated the comment for clarity.

mattwynne commented 3 years ago

@jbpros I think this is done in 57aaa63. I kept the POST /repos endpoint but if you post here now, you just pass the remoteUrl and it will generate a repoId for you, then pass you back the URL in the Link header. I used the item rel for this.

It now returns a 201 created. The redirect stuff is gone.

WDYT?