Skip to content
This repository has been archived by the owner on Oct 27, 2022. It is now read-only.

Fix nasty 500 error on POST to /_config #21

Merged
merged 2 commits into from
Nov 13, 2018
Merged

Fix nasty 500 error on POST to /_config #21

merged 2 commits into from
Nov 13, 2018

Conversation

wenli200133
Copy link
Contributor

@wenli200133 wenli200133 commented Nov 8, 2018 •
edited
Loading

Overview

When the ini config file is read only, function of
file:write_file(File, NewFileContents). doesn't return ok, so for here we should handle the return error scenario.

Testing recommendations

wenwl@wenwl_mbp:~/cloudant/couchdb$ chmod a-w ./dev/lib/node1/etc/local.ini
wenwl@wenwl_mbp:~/cloudant/couchdb$ curl -X PUT localhost:15984/_node/_local/_config/admins/joan1 -d '"500"' -u adm:pass
{"error":"bad_request","reason":"eacces"}
wenwl@wenwl_mbp:~/cloudant/couchdb$ curl -X DELETE localhost:15984/_node/_local/_config/admins/joan1 -u adm:pass
{"error":"bad_request","reason":"eacces"}

Related Issues or Pull Requests

This PR and apache/couchdb#1717 is for fixing issue of apache/couchdb#1380
PR apache/couchdb#1717 is dependent on this.
so this PR should be first merged into master, then apache/couchdb#1717 can pick it up.

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;

@wenli200133 wenli200133 mentioned this pull request Nov 8, 2018
Merged
3 tasks
jiangphcn
jiangphcn reviewed Nov 13, 2018
View reviewed changes
Copy link
Contributor

@jiangphcn jiangphcn left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions and see some minor comments.

src/config.erl Outdated
Event = {config_change, Sec, Key, Val, Persist},
gen_event:sync_notify(config_event, Event),
{reply, ok, Config};
{error, _Else} ->
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use _ before Else if _Else is used in clause

src/config.erl Outdated
Event = {config_change, Sec, Key, deleted, Persist},
gen_event:sync_notify(config_event, Event),
{reply, ok, Config};
_Else ->
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use _ before Else if _Else is used in clause

src/config.erl
end;

handle_call({delete, Sec, Key, Persist, Reason}, _From, Config) ->
true = ets:delete(?MODULE, {Sec,Key}),
couch_log:notice("~p: [~s] ~s deleted for reason ~p",
[?MODULE, Sec, Key, Reason]),
case {Persist, Config#config.write_filename} of
ConfigDeleteReturn = case {Persist, Config#config.write_filename} of
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on delete scenario

test/config_tests.erl Outdated
@@ -299,6 +299,45 @@ config_notifier_behaviour_test_() ->
}.


config_access_right_test_() ->
{
"Config file access right tests",
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow with other test description and what about Test config file access right?

@jiangphcn
Copy link
Contributor

+1 looks good from my side. CI should be fine when #22 is merged.
@wohali, hey Joan, would you please help take a look?

@wohali
Copy link
Member

wohali commented Nov 13, 2018

@jiangphcn you'll need to push a trivial commit to this branch to force travis to re-run the test.

@jiangphcn
Copy link
Contributor

@wohali yes, just doing this with @wenli200133 and rebased the change on #22

@jiangphcn
Copy link
Contributor

CI is happy now :-)

@jiangphcn
Copy link
Contributor

I will merge this PR if there is no additional comments. @wohali

@wohali
Copy link
Member

wohali commented Nov 13, 2018

You're the one who gave it a +1 @jiangphcn ;) I'm not reviewing the code here, just helping you with the process!

@asfgit asfgit merged commit 1cc8bdf into apache:master Nov 13, 2018
@jiangphcn
Copy link
Contributor

@wohali thanks for your help on process. Just merged

@jiangphcn jiangphcn mentioned this pull request Nov 26, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jiangphcn jiangphcn jiangphcn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wenli200133 @jiangphcn @wohali @asfgit