feat: Add customizable log level for 'username already exists' error

[KrishDave1]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

#9329

Closes: #9329

Approach

So I have created a new interface LogEventsOptions interface in src/Options/index.js to incorporate the custom logEvents option in case of username already taken error.

Then I have modified the RestWrite.js file to check for and apply the custom log level when a username already exists error occurs.I have also added documentation for the new option in src/Options/docs.js.

I have also included a new test case in spec/ParseUser.spec.js to verify the functionality of the custom log level.

If any changes are required , please inform me , I will do it.

Tasks

• Add tests
• Add changes to documentation (guides, repository pages, code comments)

[parse-github-assistant]

Thanks for opening this pull request!

[KrishDave1]

Thanks for the suggestions. I have made the changes according to the suggestions. @mtrezza Please have a look and tell if anything else needs to be changed

[KrishDave1]

@mtrezza I do not have a indepth knowledge of testing, earlier when you merged alpha branch into this branch, some tests were failing.I am eager to learn, and sorry if this question sounds dumb but could I know why?

[mtrezza]

earlier when you merged alpha branch into this branch, some tests were failing.

That is because with your previous commits, the CI never ran fully. If you click on the red "X" beside your commits, you can see that only 2 CI jobs ran. My merge started a full CI run. To make sure all tests pass locally you would run the full test suite locally with npm run test. You can replace describe with fdescribe or it with fit to run only single tests or sections of tests, but a full run locally should always be done at some point because you want to make sure all tests pass.

[KrishDave1]

Hey @mtrezza thanks for explaining.I have modified the tests as you suggested.Please review it and let me know

[KrishDave1]

I have made the required changes.Please check

[KrishDave1]

Are there any more changes required? @mtrezza .Sorry for asking.

[mtrezza]

I have refactored the test to make it more robust and concise. Observations:

• There are 3 places where this error can be thrown; I have added the logger to the 2 missing places --> not sure how they are invoked, we'll likely need to add tests for test coverage.
• Added testing for log levels
  • silent which seems to be a special case in which the logger must not be called
  • undefined, i.e. default value, to make sure that if no logLevel is set, the error is still logged with default log level, which is error in this case.
• The CI only fails for logLevel error, as it's logged twice --> needs to be investigated

[mtrezza]

You requested a review but you did not push any new commit. Review feedback see #9336 (comment).

[KrishDave1]

I believe while we create a user in RestWrite.js we already check for usernamealreadyexists and log it there so , no need of logging it again.Please check for the change I did.

[mtrezza]

That may be right. Why is it throwing the specific error there then? Is that error internally caught?

[KrishDave1]

Why are the tests failing?I read the test error.Isn't this what we are supposed to do?

[mtrezza]

If you run the test locally, does it pass? I assume this is only related to one of the log levels?

[mtrezza]

Good catch in 386d2c1; let's see if the whole CI passes.

[KrishDave1]

Why is the CI failing even though the logic seems correct?

[mtrezza]

I have refactored the test to make it easier to debug.

• The only log levels that are recognized by the logger are info, warn, error.
• The Parse Server config allows to also set undefined and silent as log level, but they are not real log levels interpreted by the logger. Instead, undefined means the default log level (error in this case) will be used; silent means that the logger will not be invoked at all.

To debug the test easier, I have modified it to only run with usernameAlreadyExists: 'info'. The refactored test will now check which one of the internal logger methods info, warn, error have been called. The result is that even with log level info, an event of level error is logged.

The reason seems to be that the error is logged here:

parse-server/src/RestWrite.js

Lines 742 to 745 in 714acaa

	throw new Parse.Error(
	Parse.Error.USERNAME_TAKEN,
	'Account already exists for this username.'
	);

None of the other 2 places where this PR currently places the log condition is called.

It seems this is then passed up the chain and sent to the logger at some point in the code. I believe the solution is to find that point in code where it's sent to the logger, test for the error code (and maybe error text :-/ ) and put a condition there to log accordingly.

[KrishDave1]

I think just checking what type of log levels is there should solve the issue.Please merge to check again with CI

[mtrezza]

Could you please run the test locally to see whether it passes? I doubt that this fixes it, because when I debugged the test, it didn't even run over these lines, see my comment above. Also, we probably shouldn't use this logic because there may be more or different logic level in the future. In fact, there is also a debug level, so I'd revert your last commit.

[KrishDave1]

Could you help you here I am unable to find the error.Could you help you here?Thanks

[KrishDave1]

I am a bit stuck here.Could you help me out here?I am unable to understand where is this log of error is coming from.

[KrishDave1]

The tests are still failing.Is there anything I can do?

[mtrezza]

This would need to be investigated; unfortunately I don't have the resources at the moment to support beyond the investigation I've posted above.