Skip to content

fix: Return 404 status on non-existent API calls #6296

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

Merged
merged 2 commits into from
Sep 1, 2020
Merged

Conversation

vmrajas
Copy link
Contributor

@vmrajas vmrajas commented Aug 27, 2020

Motivation

Currently, making an API call to a non-existent path on alpha HTTP server such as "/admin/test" returns status as 200 (OK). As the path does not exist, status should be 404.
Related discuss issue: https://discuss.dgraph.io/t/dgraph-should-return-404-on-non-existent-api-paths/9574

Components affected by this PR
Dgraph alpha http server

Does this PR break backwards compatibility?:
No

Testing
Added two unit tests in dgraph/cmd/alpha/http_test.go .

This Pull Request fixes this.

Fixes
DGRAPH-2335


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2020

CLA assistant check
All committers have signed the CLA.

@vmrajas vmrajas changed the title Add feature to return 404 status on non-existent API calls fix: Return 404 status on non-existent API calls Aug 28, 2020
@vmrajas vmrajas marked this pull request as ready for review August 28, 2020 10:53
Copy link

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @harshil-goel, @manishrjain, @vmrajas, and @vvbalaji-dgraph)


dgraph/cmd/alpha/http_test.go, line 941 at r1 (raw file):

	resp, err := client.Do(req)
	require.NoError(t, err)
	require.True(t, resp.StatusCode == 404)

Use require.Equal()


dgraph/cmd/alpha/http_test.go, line 942 at r1 (raw file):

	require.NoError(t, err)
	require.True(t, resp.StatusCode == 404)
	require.True(t, resp.Status == "404 Not Found")

Use require.Equal()

Copy link
Contributor Author

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @harshil-goel, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/alpha/http_test.go, line 941 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Use require.Equal()

Done.


dgraph/cmd/alpha/http_test.go, line 942 at r1 (raw file):

Previously, arijitAD (Arijit Das) wrote…

Use require.Equal()

Done.

@vmrajas vmrajas requested a review from arijitAD August 28, 2020 14:14
Copy link
Contributor Author

@vmrajas vmrajas left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @harshil-goel, @manishrjain, and @vvbalaji-dgraph)

Copy link

@arijitAD arijitAD left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @harshil-goel, @manishrjain, and @vvbalaji-dgraph)

@vmrajas vmrajas merged commit 092a884 into master Sep 1, 2020
@vmrajas vmrajas deleted the rajas/return_404 branch September 1, 2020 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants