Skip to content

Conversation

@coratgerl
Copy link
Contributor

@coratgerl coratgerl commented Dec 6, 2025

Pull Request

Issue

Fixes: #9329

  • Add tests
  • Add changes to documentation (guides, repository pages, code comments)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK

Summary by CodeRabbit

  • New Features

    • Added a new log-level setting signupUsernameTaken to control how sign-up failures for an already-existing username are logged (default: 'error'); documentation updated.
  • Tests

    • Added tests that verify logging behavior for duplicate-username signups under different log-level configurations (e.g., 'warn' and 'silent').

✏️ Tip: You can customize this high-level summary in your review settings.

@parse-github-assistant
Copy link

🚀 Thanks for opening this pull request!

@coderabbitai
Copy link

coderabbitai bot commented Dec 6, 2025

📝 Walkthrough

Walkthrough

Adds a new log-level option signupUsernameTaken (configurable via env PARSE_SERVER_LOG_LEVELS_USERNAME_ALREADY_EXISTS) and updates middleware to honor it when a signup fails with Parse.Error.USERNAME_TAKEN. Adds tests covering 'warn' and 'silent' behaviors and updates docs/types/definitions.

Changes

Cohort / File(s) Summary
Tests
spec/ParseUser.spec.js
Adds two tests that attempt duplicate-username signups and assert logger calls for signupUsernameTaken at 'warn' and 'silent' settings.
Configuration definitions & docs
src/Options/Definitions.js, src/Options/docs.js, src/Options/index.js, types/Options/index.d.ts
Introduces signupUsernameTaken in LogLevels with env PARSE_SERVER_LOG_LEVELS_USERNAME_ALREADY_EXISTS, default 'error', plus documentation and TypeScript typing.
Error handling middleware
src/middlewares.js
In handleParseErrors, reads req.config?.logLevels?.signupUsernameTaken and logs Parse.Error.USERNAME_TAKEN using that level (treating 'silent' as no-op and falling back to log.error if level method missing).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

  • Areas to inspect:
    • src/middlewares.js — ensure correct handling of the 'silent' level, fallback when the configured level isn't a logger method, and no change to HTTP response behavior.
    • Consistent naming of the new option (signupUsernameTaken) across Definitions, docs, index, and type declarations.
    • Tests in spec/ParseUser.spec.js — validate assertions correctly reflect logger call counts and error outcomes.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature: adding an option to customize log levels for username already exists messages.
Description check ✅ Passed The PR description follows the template with issue reference (#9329), test checklist marked complete, and properly formatted contribution guidelines.
Linked Issues check ✅ Passed The PR implementation fulfills the core requirement from #9329: it adds a configurable log level option (signupUsernameTaken) for username-taken signup errors, allowing customization including silencing.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the requested feature: LogLevels configuration, middleware error handling, tests, and type definitions for the new option.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5087a15 and b71c7d8.

📒 Files selected for processing (6)
  • spec/ParseUser.spec.js (1 hunks)
  • src/Options/Definitions.js (1 hunks)
  • src/Options/docs.js (1 hunks)
  • src/Options/index.js (1 hunks)
  • src/middlewares.js (2 hunks)
  • types/Options/index.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Options/Definitions.js
  • spec/ParseUser.spec.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.

Applied to files:

  • src/Options/docs.js
  • src/Options/index.js
🪛 Biome (2.1.2)
src/Options/index.js

[error] 792-792: Expected a statement but instead found '?'.

Expected a statement here.

(parse)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
  • GitHub Check: Node 22
  • GitHub Check: Node 18
  • GitHub Check: Node 20
  • GitHub Check: Redis Cache
  • GitHub Check: MongoDB 8, ReplicaSet
  • GitHub Check: MongoDB 7, ReplicaSet
  • GitHub Check: PostgreSQL 15, PostGIS 3.3
  • GitHub Check: PostgreSQL 17, PostGIS 3.5
  • GitHub Check: PostgreSQL 15, PostGIS 3.5
  • GitHub Check: PostgreSQL 16, PostGIS 3.5
  • GitHub Check: MongoDB 6, ReplicaSet
  • GitHub Check: PostgreSQL 18, PostGIS 3.6
  • GitHub Check: PostgreSQL 15, PostGIS 3.4
  • GitHub Check: Benchmarks
🔇 Additional comments (5)
src/Options/docs.js (1)

331-331: LGTM!

The documentation for the new log level is clear and follows the established pattern for other log level properties.

src/middlewares.js (2)

485-495: LGTM!

The conditional logging implementation is well-structured:

  • Correctly checks for USERNAME_TAKEN error code
  • Respects the 'silent' setting by not logging
  • Defensively checks if the log level method exists before calling it
  • Falls back to log.error if the method is not available
  • Preserves existing behavior for all other Parse errors

469-470: Verify the fallback default value for consistency.

The fallback default in src/middlewares.js (line 470) is set to 'info', but the review indicates documentation may state the default is 'error'. Ensure the fallback value matches the actual default specified in src/Options/Definitions.js and that all three configuration files (index.js, Definitions.js, docs.js) are in sync. If changes were made to index.js, confirm that npm run definitions was executed to update the other files.

types/Options/index.d.ts (1)

300-300: LGTM!

The TypeScript type definition is correct and consistent with the other log level properties in the interface.

src/Options/index.js (1)

792-795: Verify the static analysis error and confirm npm run definitions execution.

The addition follows the correct pattern for Flow type definitions. However, Biome is reporting a parse error on line 792—this might be a false positive due to Flow syntax not being fully supported by Biome, but please verify that the code compiles correctly.

Ensure that npm run definitions has been executed after modifying src/Options/index.js. This command automatically updates src/Options/Definitions.js and src/Options/docs.js with the new signupUsernameTaken entry.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.40.0)
spec/ParseUser.spec.js

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@parseplatformorg
Copy link
Contributor

parseplatformorg commented Dec 6, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 6, 2025
@codecov
Copy link

codecov bot commented Dec 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.58%. Comparing base (e78e58d) to head (b71c7d8).

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #9962      +/-   ##
==========================================
+ Coverage   92.56%   92.58%   +0.01%     
==========================================
  Files         191      191              
  Lines       15544    15549       +5     
  Branches      177      177              
==========================================
+ Hits        14389    14396       +7     
+ Misses       1143     1141       -2     
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mtrezza
Copy link
Member

mtrezza commented Dec 7, 2025

I think we can also the default level for this to info. It's not really an error or warning.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 8, 2025
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

LogEvents is still all over the code

@coratgerl
Copy link
Contributor Author

LogEvents is still all over the code

Do you mean this object ?
image

This is because in the issue you mentionned this kind of object:
image

Do you want I delete the object and pass directly the option ?

@mtrezza
Copy link
Member

mtrezza commented Dec 8, 2025

I see where you got this from. May have been a typo. logLevels does the same for Cloud Function and trigger events:

logLevels: {
  env: 'PARSE_SERVER_LOG_LEVELS',
  help: '(Optional) Overrides the log levels used internally by Parse Server to log events.',
  action: parsers.objectParser,
  type: 'LogLevels',
  default: {},
},

Let's add there, unless you can think of a reason not to? They are grouped by prefix, cloudFunction..., trigger..., so maybe add a prefix like signup... since this is a sign-up event and name it signupUsernameTaken, which is the same wording as ParseError.USERNAME_TAKEN = 202, so we have a nice mapping.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
types/Options/index.d.ts (1)

1-126: PR title suggestion in Angular convention

To align with the repo’s preferred Angular-style convention, consider updating the PR title to something like:

feat(logging): configurable log level for username-taken signups

This keeps the existing intent but adds a clear scope (logging) and a concise, action-oriented description.

Based on learnings, this helps generate a clearer changelog entry.

src/Options/index.js (1)

792-795: Clarify intended default and naming for logLevels.usernameAlreadyExists

The new option is wired and documented as:

  • Comment + :DEFAULT: say default is 'error'.
  • Type: usernameAlreadyExists: ?string;.

From the linked issue, there was discussion about:

  • Changing the default behavior to something less noisy (e.g. info), and
  • A potential name like signupUsernameTaken to align with Parse.Error.USERNAME_TAKEN.

Can you double-check that:

  1. The intended default log level for this event (for fresh installs and existing deployments) is indeed 'error', and that this matches:

    • The default in src/Options/Definitions.js for LogLevels.usernameAlreadyExists, and
    • The fallback used in the middleware when no explicit value is configured.
  2. The final option name usernameAlreadyExists is the agreed one vs. signupUsernameTaken, and if not, rename it consistently across:

    • types/Options/index.d.ts
    • src/Options/index.js
    • src/Options/Definitions.js
    • Any runtime usage (e.g. req.config.logLevels.usernameAlreadyExists in middleware, tests).

Aligning these keeps behavior, documentation, and configuration introspection in sync.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4526b0a and 5087a15.

📒 Files selected for processing (6)
  • spec/ParseUser.spec.js (1 hunks)
  • src/Options/Definitions.js (1 hunks)
  • src/Options/docs.js (1 hunks)
  • src/Options/index.js (1 hunks)
  • src/middlewares.js (2 hunks)
  • types/Options/index.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/middlewares.js
  • src/Options/docs.js
  • spec/ParseUser.spec.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.

Applied to files:

  • src/Options/Definitions.js
  • src/Options/index.js
🪛 Biome (2.1.2)
src/Options/index.js

[error] 792-792: Expected a statement but instead found '?'.

Expected a statement here.

(parse)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
  • GitHub Check: PostgreSQL 15, PostGIS 3.3
  • GitHub Check: PostgreSQL 16, PostGIS 3.5
  • GitHub Check: PostgreSQL 18, PostGIS 3.6
  • GitHub Check: Node 20
  • GitHub Check: MongoDB 7, ReplicaSet
  • GitHub Check: PostgreSQL 15, PostGIS 3.4
  • GitHub Check: PostgreSQL 17, PostGIS 3.5
  • GitHub Check: MongoDB 8, ReplicaSet
  • GitHub Check: Node 22
  • GitHub Check: Redis Cache
  • GitHub Check: MongoDB 6, ReplicaSet
  • GitHub Check: PostgreSQL 15, PostGIS 3.5
  • GitHub Check: Node 18
  • GitHub Check: Docker Build
  • GitHub Check: Benchmarks
🔇 Additional comments (2)
types/Options/index.d.ts (1)

294-301: LogLevels typings for usernameAlreadyExists look consistent

usernameAlreadyExists?: string; matches the pattern of existing LogLevels properties and aligns with the new runtime option name; no typing issues here.

src/Options/Definitions.js (1)

1479-1515: Definitions entry for usernameAlreadyExists is consistent

The new module.exports.LogLevels.usernameAlreadyExists entry (env name, help text, and default: 'error') is consistent with the documentation in src/Options/index.js and with the existing LogLevels structure.

Once you confirm the intended default level and naming in src/Options/index.js, this definition will already be aligned.

coderabbitai[bot]
coderabbitai bot previously approved these changes Dec 9, 2025
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.

Add log option for username taken event

3 participants