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

ReleasesClient.GenerateReleaseNotes uses the wrong URL and HTTP method #2589

Closed
rdeago opened this issue Oct 6, 2022 · 2 comments · Fixed by #2592
Closed

ReleasesClient.GenerateReleaseNotes uses the wrong URL and HTTP method #2589

rdeago opened this issue Oct 6, 2022 · 2 comments · Fixed by #2592
Assignees
Labels
Type: Bug Something isn't working as documented

Comments

@rdeago
Copy link
Contributor

rdeago commented Oct 6, 2022

Both overloads of ReleasesClient.GenerateReleaseNotes have incorrect routes.

EDIT: They also use the GET method instead of POST.


[ManualRoute("GET", "/repos/{owner}/{repo}/releases")]
public Task<GeneratedReleaseNotes> GenerateReleaseNotes(string owner, string name, GenerateReleaseNotesRequest data)

Here, the route is missing /generate-notes at the end, as per REST API docs.

What's funny is that all parameters to the .../releases/generate-notes entry point are also valid parameters to the .../releases entry point, thus the HTTP request returns 200 OK.

Furthermore, the .../releases response contains, among others, all fields of the .../releases/generate-notes response and is thus successfully deserialized to GeneratedReleaseNotes.

The end result is that the method succeeds, even if the outcome is far from what one may expect: a new release is created and published, triggering validation errors in a later call to either ReleasesClient.Create or ReleasesClient.Edit with the same tag name.

By the way, the route is wrong in unit tests, too:

client.Received().Post<GeneratedReleaseNotes>(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/releases"),
data,
"application/vnd.github.v3");


[ManualRoute("GET", "/repos/{owner}/{repo}/releases")]
public Task<GeneratedReleaseNotes> GenerateReleaseNotes(long repositoryId, GenerateReleaseNotesRequest data)

This route is missing /generate-notes at the end too, plus it doesn't even match method parameters, as it has /repos/{owner}/{repo} instead of /repositories/{id}.

I haven't experimented with this one, but I strongly suspect it throws.

The unit test here tests the right route though (minus /generate-notes):

client.Received().Post<GeneratedReleaseNotes>(Arg.Is<Uri>(u => u.ToString() == "repositories/1/releases"),
data,
"application/vnd.github.v3");

How can the test pass? It may be related to how method parameters are mapped to ManualRoute attributes, but I'm just guessing here.

EDIT: Turns out ManualRoute attributes are actually not used to generate URLs: static methods of class ApiUrls are called instead. GenerateReleaseNotes methods call ApiUrls.Releases methods, which construct URLs suitable for listing releases, not for generating release notes. What's missing are two ApiUrls.ReleasesGenerateNotes methods (one for owner / repo, the other for repository ID) that return URLs with /generate-notes appended.


Please forgive me if I came out as blunt: it is not in my intentions to offend anyone. I think @Cristaling did a great job with the GenerateReleaseNotes implementation, by the way. I was just trying to be concise, fighting against my over-verbose Italian nature, 😄 and I realized later I might have erred on the other side.

How about I open a PR for this?

@JonruAlveus
Copy link
Collaborator

Hi @rdeago, thanks for raising the issue. If you could raise a PR for this, that would be great otherwise we can pick it up soon.

@rdeago rdeago changed the title ReleasesClient.GenerateReleaseNotes calls the wrong URL ReleasesClient.GenerateReleaseNotes uses the wrong URL and HTTP method Oct 7, 2022
@rdeago
Copy link
Contributor Author

rdeago commented Oct 7, 2022

Thanks for the prompt answer. PR incoming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants