Modernization improvements: validation, logging, retry logic, and error handling by heywinit · Pull Request #46 · niledatabase/cli

heywinit · 2026-03-25T19:21:36Z

PR Overview
This PR adds several infrastructure improvements to make the CLI more robust: input validation utilities, structured logging, API retry logic for resilience, and consolidated error handling.

Follow up to #45

Changes

Input Validation (src/lib/validation.ts - new)
- validateDatabaseName() - validates DB names follow PostgreSQL naming rules (63 chars max, lowercase letters/numbers/underscores/hyphens, no reserved words)
- validateTenantName() - validates tenant names (255 char max)
- validateTenantId() - validates tenant IDs
Structured Logging (src/lib/logger.ts - new)
- info(), debug(), warn(), error() functions with timestamp, level, and message formatting
- Debug mode support for verbose output
API Retry Logic (src/lib/api.ts)
- Added axios-retry with exponential backoff (3 retries)
- Retries on 408, 429, and 5xx errors
- Debug logging for retry attempts
Error Handling Refactor (src/lib/errorHandling.ts)
- Consolidated multiple error handlers into single handleError() function
- Added ErrorContext type for context-aware error messages (API, Database, Tenant, User)
- Cleaned up redundant code and comments
Lint Fixes (src/commands/local.ts, .eslintrc.js)
- Removed unused error variables in catch blocks

…plane)

gwenshap

Thanks for all the validations. Overall, it looks like good improvements and we appreciate.
I had few comments around retries and validations.

gwenshap · 2026-03-27T01:45:14Z

src/lib/api.ts

+        retryDelay: axiosRetry.exponentialDelay,
+        retryCondition: (error) => {
+          const status = error.response?.status;
+          return !status || status === 408 || status === 429 || (status >= 500 && status < 600);


Any reason not to stick to the default retry condition for axiosRetry (isNetworkOrIdempotentRequestError)?

It seems safer (since error 500 is not always safe to retry). As far as I can see, the only status your code handles and isNetworkOrIdempotentRequestError does not is 408, and Nile doesn't return that.

Fixed it using isNetworkOrIdempotentRequestError now

gwenshap · 2026-03-27T01:50:28Z

src/lib/validation.ts

+    return { valid: false, error: 'Database name is required' };
+  }
+
+  if (name.length > 63) {


We actually don't limit database names in Nile

gwenshap · 2026-03-27T01:55:03Z

src/lib/validation.ts

+  }
+
+  if (!/^[a-z][a-z0-9_-]*$/.test(name)) {
+    return { valid: false, error: 'Database name must start with lowercase letter and contain only lowercase letters, numbers, underscores, and hyphens' };


we don't allow hyphens

gwenshap · 2026-03-27T01:55:19Z

src/lib/validation.ts

+  }
+
+  if (name.length > 255) {
+    return { valid: false, error: 'Tenant name must be 255 characters or less' };


We allow any length

gwenshap · 2026-03-27T01:56:40Z

src/lib/validation.ts

+  }
+
+  if (email.length > 255) {
+    return { valid: false, error: 'Email must be 255 characters or less' };


We don't limit email length

.eslintrc.js

heywinit · 2026-03-27T17:21:23Z

Thanks for the quick review. My bad I wasn't aware of Nile's validation scope. I've fixed everything in fd878fc. Let me know if there are any other caveats I should take care of as well.

gwenshap · 2026-04-03T00:11:51Z

I just realized that our CI/CD wasn't set to run the test action for external contributions. I fixed that in a separate PR.

But, when I tested your PR locally (with npm test), I noticed 3 test failures that appear related to your changes.
Do you mind taking a look and fixing them? Hopefully the next time you add a commit to this branch, the test action will trigger and also pass :)

heywinit added 6 commits March 25, 2026 02:32

refactor: consolidate error handling into single generic handler

6cb82b0

feat: add axios retry logic for API resilience

3b03a86

feat: add input validation and structured logging utilities

2de660d

fix: remove unused imports and variables

83237b0

chore(validation): remove strict region validation (defer to control …

15c3038

…plane)

chore(validation): remove validateRegion implementation and references

362899c

gwenshap requested changes Mar 27, 2026

View reviewed changes

gwenshap reviewed Mar 27, 2026

View reviewed changes

.eslintrc.js Show resolved Hide resolved

realign validation after feedback

fd878fc

Conversation

heywinit commented Mar 25, 2026

Changes

Uh oh!

gwenshap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

heywinit commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gwenshap commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

heywinit commented Mar 27, 2026 •

edited

Loading