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

fs rc: fixes incorrect Content-Type in HTTP API - fixes #7726 #7730

Merged
merged 1 commit into from Apr 13, 2024

Conversation

KDreynolds
Copy link
Contributor

What is the purpose of this change?

Was the change discussed in an issue or in the forum before?

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

I think the error case needs fixing though and needs a test - see inline.

Formatting good according to the CI.

Thank you :-)

fs/rc/rcserver/rcserver.go Show resolved Hide resolved
tests := []testRun{
{
Name: "Check Content-Type for JSON response",
URL: "rc/noop", // Assuming rc/noop returns JSON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to test the error case with rc/error - I'd suggest another test for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I added another test case for rc/error, good chance I did something in an odd way, lmk

@KDreynolds KDreynolds force-pushed the issue7726 branch 2 times, most recently from 92b478d to efcdb43 Compare April 8, 2024 14:33
Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :-)

Please see inline comments.

testServer(t, tests, &opt)
}

func TestErrorContentTypeJSON(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this test just be another test case in TestContentTypeJSON? I'm probably missing something as to why not...

If not I suggest you add a new feature to the testServer rather than duplicating it below. By that I mean add a new flag to testRun, set it in the test case, and use it to control the changes you need in testServer. That will make for much neater and more maintainable code.

// Simulate setting the error response in your actual handler
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(tc.ExpectedStatus)
w.Write([]byte(tc.ExpectedJSON)) // This should be replaced with the actual error handling logic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter says this is missing an error check. w.Write returns a length and error if I remember correctly, so you'd check it something like this

_, err := w.Write(...)
require.NoError(t, err)

@KDreynolds
Copy link
Contributor Author

not exactly sure why the mac amd build failed.
Hopefully these changes work and are coherent.
Thanks!

Copy link
Member

@ncw ncw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks very nice now.

I like your approach normalizing the JSON.

The mac test failed due to a known flakey test not your code, so I will merge this now thank you :-)

@ncw ncw merged commit 47cbddb into rclone:master Apr 13, 2024
9 of 10 checks passed
@KDreynolds
Copy link
Contributor Author

Thank you! Appreciate it!

@KDreynolds KDreynolds deleted the issue7726 branch April 15, 2024 00:26
@ncw
Copy link
Member

ncw commented Apr 15, 2024

@KDreynolds I appreciate your contributions and I enjoy helping you with them, so keep them coming :-)

@nullinger
Copy link

Thank you, @KDreynolds and @ncw !

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

Successfully merging this pull request may close these issues.

None yet

3 participants