feat: add MongoDB support with CRUD operations #108
feat: add MongoDB support with CRUD operations #108Youssef-joe wants to merge 12 commits intostagefrom
Conversation
…107) - Implemented MongoDB query execution with `executeMongoQuery`. - Added functions for managing records in MongoDB (`addMongoRecord`, `updateMongoRecords`, `deleteMongoRecords`, etc.). - Created schema retrieval for MongoDB databases with `getMongoDatabaseSchema`. - Developed table management functions for MongoDB, including `getMongoTablesList`, `createMongoCollection`, and `deleteMongoColumn`. - Enhanced error handling to include MongoDB-specific errors. - Updated routes to support MongoDB operations alongside existing PostgreSQL functionality. - Modified database URL parsing to accommodate MongoDB connection strings. - Expanded system prompt generation to include MongoDB query guidelines. - Updated shared types to include MongoDB as a valid database type.
Youssef-joe
changed the title
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces MongoDB database support to the application, adding development orchestration scripts, a MongoDB client manager, data access objects for MongoDB operations (queries, records, tables, schema), frontend language/type awareness, database store synchronization, and conditional routing in server endpoints between PostgreSQL and MongoDB implementations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/server/src/db-manager.ts (2)
50-58:⚠️ Potential issue | 🟡 MinorPort defaults to 5432 (PostgreSQL) for MongoDB connections.
Line 54 falls back to port
5432when the URL has no explicit port. Formongodb+srv://URLs, there's typically no port in the URL (DNS SRV handles it), so this will incorrectly store5432. While this config may not be used for MongoDB connections directly, it's misleading and could cause bugs if referenced.Suggested fix
- port: Number.parseInt(url.port, 10) || 5432, + port: Number.parseInt(url.port, 10) || (this.detectDbType(url) === "mongodb" ? 27017 : 5432),
127-133:⚠️ Potential issue | 🟠 MajorConnection strings (potentially containing credentials) are logged to console.
Lines 127 and 133 log full connection strings which may contain passwords. Consider redacting credentials before logging.
🤖 Fix all issues with AI agents
In `@kill-dev.sh`:
- Around line 32-34: Remove the stray leading spaces on the top-level tokens so
indentation is consistent: locate the lines containing the literal token done
and the line echo "[kill-dev] done" and left-align them (no leading space) to
match the rest of the script's top-level statements.
In `@packages/core/src/components/runnr-tab/cdoe-editor.tsx`:
- Around line 42-230: The effect registers providers with monaco but never
disposes them, causing duplicates; capture the IDisposables returned by
monaco.languages.registerDocumentFormattingEditProvider and
monaco.languages.registerCompletionItemProvider (e.g., formattingDisposable and
completionDisposable) when you register the providers around
provideDocumentFormattingEdits and provideCompletionItems, then return a cleanup
function from the effect that calls dispose() on each disposable (guarding for
existence) so providers are removed on effect teardown or when language changes.
In `@packages/core/src/hooks/use-databases-list.ts`:
- Line 55: The hook useCurrentDatabase destructures setDbType from
useDatabaseStore but never calls it, causing the store dbType to stay out of
sync with the API client; update useCurrentDatabase so that whenever
setApiDbType(...) is invoked (the same place where the API client's base URL is
changed), also call setDbType(...) with the same db type value; ensure you
reference/modify the block that currently calls setApiDbType and add a call to
setDbType there so components reading useDatabaseStore().dbType (e.g.,
SidebarSearchTables) receive the updated type.
In `@packages/server/package.json`:
- Line 60: Update the package metadata to reflect MongoDB support: change the
"description" field to mention MongoDB alongside PostgreSQL, add "mongodb" and
"mongo" to the "keywords" array, and either bump the "mongodb" dependency from
"mongodb": "^6.19.0" to the current stable "^7.1.0" if your code is compatible
or leave the version and add a TODO comment in package metadata noting the
intentional pin for compatibility; target the "description", "keywords", and the
"mongodb" dependency entries to make these changes.
In `@packages/server/src/dao/mongo/database-list.dao.ts`:
- Around line 54-62: The returned object's max_connections is using
serverStatus.connections.available (remaining slots) instead of the real max;
update the code that builds the return object (the block returning
host/port/user/database/version/active_connections/max_connections) to compute
max_connections as (serverStatus.connections?.current ?? 0) +
(serverStatus.connections?.available ?? 0) so the true maximum is shown; keep
active_connections as serverStatus.connections?.current ?? 0 and preserve use of
urlDefaults and getMongoDbName().
In `@packages/server/src/dao/mongo/query.dao.ts`:
- Around line 117-131: The current handlers for updateOne/updateMany set
rowCount = result.modifiedCount which hides cases where documents matched but
weren't modified; change the assignment to use result.matchedCount (e.g.,
rowCount = result.matchedCount ?? 0) and also surface the modified count by
adding a separate modifiedCount field or appending it to message so callers can
distinguish "matched but not modified" from "no match"; update the code paths
around updateOne/updateMany, rowCount, result.modifiedCount, result.matchedCount
and the response construction to include both matchedCount and modifiedCount.
- Around line 83-93: The call to buildMongoSortForQuery using an empty string as
the first arg is unclear and results in a default { _id: 1 }; change the call in
the "find" case to pass undefined (or null) for the field parameter so intent is
explicit: replace buildMongoSortForQuery("", undefined) with
buildMongoSortForQuery(undefined, undefined) (or
buildMongoSortForQuery(undefined)) and ensure the local variable sort (used in
cursor.sort(sort)) remains the same; reference payload.operation === "find", the
sort variable, and buildMongoSortForQuery when making the change.
- Around line 28-45: normalizeIdFilter currently only converts _id when it's a
plain string or inside a $in array; update it to recursively walk the filter
object (handle logical operators and nested objects) and coerce any string
values intended as ObjectId using toMongoId. Specifically, enhance
normalizeIdFilter to: 1) convert string _id values for operators $ne, $nin, $not
(and $in) to ObjectId; 2) map arrays under $in/$nin to toMongoId for string
elements; and 3) descend into nested logical operators ($and, $or, $nor, $not)
and apply the same conversions to any _id occurrences. Keep using the existing
toMongoId helper and ensure the function returns a new normalized filter object
without mutating the original.
In `@packages/server/src/dao/mongo/schema.dao.ts`:
- Around line 17-27: The getMongoDatabaseSchema function accepts maxTables but
never uses it; modify getMongoDatabaseSchema to limit the collections processed
by applying the maxTables value (e.g., if options.maxTables is set, take only
the first maxTables entries from the collections list or call
listCollections().limit(maxTables) before toArray()) so you don't fetch/process
all collections in parallel; update the code path that iterates over the
collections (the collections variable and any downstream mapping that loads
sample data when includeSampleData is used) to use the truncated collection list
so maxTables correctly bounds work and memory usage.
In `@packages/server/src/dao/mongo/tables.dao.ts`:
- Around line 86-94: The parseValue function returns the original untrimmed raw
string on the fallback path which preserves whitespace inconsistently; update
parseValue (the function using trimmed, Number(), isValidObjectId and
coerceObjectId) to return trimmed instead of raw for the final fallback so plain
string filter values don't retain leading/trailing whitespace.
- Around line 64-81: The switch in mapDataTypeLabel has inconsistent indentation
for the "json" and "date" cases; update the indentation of those case lines and
their returns to match the other cases (same indentation level as "number",
"boolean", "array", "enum") so the entire switch in function mapDataTypeLabel
(returning ColumnInfoSchemaType["dataTypeLabel"]) is uniformly indented and
formatted.
- Around line 215-228: The code only marks columns nullable when a value is
explicitly null/undefined; update the logic after the documents loop to mark
columns as nullable if they are absent in any document: iterate over documents
and maintain a count or a Set of seen keys, then for each key in columnMap
(refer to columnMap, documents, and ensureColumn) set nullable=true when its
seen-count < documents.length (i.e., missing in some docs); this ensures
sparse/missing fields are flagged nullable in the generated metadata.
- Around line 83-148: Summary: Regex injection risk in buildMongoFilters where
parseValue(filter.value) is used directly for $regex in the "like"/"ilike"/"not
like"/"not ilike" branches. Fix: add an escape helper (e.g., escapeRegex) and
use it before creating any $regex condition inside buildMongoFilters; ensure the
value used for regex is coerced to a string (check typeof value === "string" or
use String(value) fallback) and escape regex metacharacters, then use the
escaped string as the $regex pattern and keep $options ("i" for ilike/not ilike,
"" for like/not like); update the cases for "like","not like","ilike","not
ilike" in buildMongoFilters and ensure parseValue remains unchanged for other
operators.
In `@packages/server/src/mongo-manager.ts`:
- Around line 34-46: The getMongoClient function currently assigns the
module-level client before awaiting connect, so a failed client.connect() leaves
a broken client cached; change the logic to only set the module-level client
after a successful connection (e.g., create a local/temp MongoClient, await
temp.connect(), then assign temp to the module-level client variable), or catch
errors from client.connect() and ensure the module-level client remains
undefined/closed on failure; update references to client, getMongoClient, and
baseConfig accordingly.
🧹 Nitpick comments (23)
kill-dev.sh (1)
24-31:sedcaptures only the lastpid=token per line.If
ssever outputs multiplepid=entries on a single line (e.g., multiple sockets sharing a port), only the last PID is extracted due to greedy.*. Usinggrep -oPwould be more robust.Proposed fix
- pids=$(ss -lptn "sport = :${port}" 2>/dev/null | sed -n 's/.*pid=\([0-9]\+\).*/\1/p') + pids=$(ss -lptn "sport = :${port}" 2>/dev/null | grep -oP 'pid=\K[0-9]+')dev.sh (1)
14-29: Backgrounded subshell PIDs may not propagate signals to child processes.
$!captures the PID of the(cd … && cmd) &subshell, not the innercmd. Killing the subshell withSIGTERMdoesn't guarantee the child (e.g.,npm run dev) is also terminated, potentially leaving orphaned processes.This is partially mitigated by the port-based cleanup in
kill-dev.sh, but you could make the cleanup more reliable by usingexecin the subshell so the inner command replaces the subshell process:Proposed fix
run() { local name="$1" shift echo "[dev] starting ${name}..." - (cd "$ROOT_DIR" && "$@") & + (cd "$ROOT_DIR" && exec "$@") & echo $! >> /tmp/db-studio-dev.pids } run_in() { local name="$1" local dir="$2" shift 2 echo "[dev] starting ${name}..." - (cd "$dir" && "$@") & + (cd "$dir" && exec "$@") & echo $! >> /tmp/db-studio-dev.pids }packages/server/package.json (1)
5-5: Updatedescriptionandkeywordsto reflect MongoDB support.The description says "Modern database client for PostgreSQL…" and the keywords are PostgreSQL/MySQL/SQLite-centric. Now that MongoDB is a supported database, consider updating these to reflect the broader scope — this matters for discoverability (npm search, GitHub).
packages/server/src/middlewares/error-handler.ts (1)
34-35: Prefer checkinge.nameinstead ofe.message.includes()for MongoDB errors.MongoDB driver errors have a
nameproperty (e.g.,"MongoNetworkError","MongoServerSelectionError"). Relying one.message.includes()is fragile — the class name isn't guaranteed to appear in the message text.Suggested improvement
const isConnectionError = e.message.includes("ECONNREFUSED") || e.message.includes("connection refused") || e.message.includes("timeout expired") || e.message.includes("Connection terminated") || - e.message.includes("MongoNetworkError") || - e.message.includes("MongoServerSelectionError") || + e.name === "MongoNetworkError" || + e.name === "MongoServerSelectionError" || (e instanceof DatabaseError && e.code?.startsWith("08")); // Connection exception classpackages/core/src/hooks/use-databases-list.ts (1)
63-83: DuplicateselectedDatabaseinitialization — both inqueryFnand theuseEffect.Lines 69-71 (inside
queryFn) and lines 77-83 (in theuseEffect) both setselectedDatabasewhen it's not already set. TheuseEffectis redundant since thequeryFnalready handles this. Having both paths makes the initialization logic harder to follow and could lead to unnecessary re-renders.Consider removing the
useEffect(lines 77-83) since thequeryFnalready handles it, or consolidate all side effects into theuseEffectand keep thequeryFnpure.packages/core/src/components/runnr-tab/cdoe-editor.tsx (1)
324-332: Callback dependencies will cause excessive editor recreation.
onQueryChange,onUnsavedChanges, andonExecuteQueryare in the dependency array. Unless every caller wraps these inuseCallback, any parent re-render will produce new function references, destroying and recreating the entire Monaco editor instance. This is a pre-existing issue, but addinglanguageto the deps is fine — the editor should indeed be recreated when the language changes.Consider using refs for the callback props to avoid tearing down the editor on every parent render, or ensure all callbacks passed to this component are memoized.
packages/server/src/mongo-manager.ts (2)
1-71: No graceful shutdown for the MongoClient.There's no exported
closeMongoClient()function. If the server shuts down (SIGTERM, hot reload in dev), the connection stays open until the process exits. Consider exporting a close helper for clean shutdown hooks.
62-64: TightenisValidObjectIdto accept only canonical 24-character hex ObjectId strings.
ObjectId.isValid()returnstruefor any 12-character string—including non-hex strings like"surveillance"(which is 12 bytes)—not just valid hex ObjectIds. Since this function is used as a type guard when coercing user-supplied IDs, enforce the canonical format:return typeof value === "string" && /^[0-9a-fA-F]{24}$/.test(value);Alternatively, validate and roundtrip:
ObjectId.isValid(value) && new ObjectId(value).toHexString() === value.packages/server/src/utils/system-prompt-generator.ts (1)
131-133: Minor:dbTypeLabelmapping is duplicated.The same
schema.dbType === "pg" ? "PostgreSQL" : schema.dbTypelogic exists at both Line 8 and Line 132. Consider extracting it into a small helper (e.g.,getDbTypeLabel(dbType)) to keep them in sync.packages/server/src/dao/mongo/records.dao.ts (3)
54-64: Update field values in$setare not coerced throughtoMongoId.The PK value is coerced to
ObjectIdon Line 69 when the field is_id, but if a user updates a field that contains an ObjectId reference (e.g., a foreign-key-like field pointing to another collection), the value inupdateSet(Line 64) is stored as a plain string. This may be acceptable for now — just flagging in case downstream consumers expectObjectIdtypes for reference fields.
67-83: SequentialupdateOnecalls — acceptable for a studio tool, but considerbulkWritefor larger batches.Each grouped PK triggers a separate
updateOneround trip. If this is only used from the UI for editing a handful of rows at a time, it's fine. For future-proofing,collection.bulkWrite()would reduce round trips.
88-103:deleteMongoRecordsassumes allprimaryKeysentries share the samecolumnName.Line 96 reads
columnNameonly from the first entry. If the array ever contains mixed column names, records for non-first columns would be silently skipped. This is likely fine given the UI sends deletes for a single collection's PK at a time, but a defensive check or assertion could prevent subtle bugs if the contract changes.packages/server/src/dao/mongo/database-list.dao.ts (1)
38-40: Nit:getMongoCurrentDatabaseis declaredasyncbut performs no async work.
getMongoDbName()is synchronous. Theasyncqualifier is unnecessary — though it's harmless since it just wraps the return in a resolved Promise.packages/server/src/routes/records.routes.ts (1)
124-128:forceDeleteMongoRecordsis a passthrough todeleteMongoRecords.Since MongoDB doesn't enforce foreign-key constraints, the "force" variant is semantically identical to the regular delete. This is fine, but consider adding a brief inline comment (here or in the DAO) to document why the two paths converge for MongoDB — it'll save future readers from wondering if it's an oversight.
packages/server/src/dao/table-details-schema.ts (2)
10-12: Merge the two@/db-manager.jsimports into one.Lines 10 and 11 both import from the same module. Consolidate them.
✏️ Proposed fix
-import { getDbPool } from "@/db-manager.js"; -import { getDbType } from "@/db-manager.js"; +import { getDbPool, getDbType } from "@/db-manager.js";
130-134: WiremaxTablesthrough togetMongoDatabaseSchemato limit collection processing.The
getMongoDatabaseSchemacall only forwardsincludeSampleData, ignoringincludeDescriptionsandmaxTables. OmittingincludeDescriptionsis intentional since MongoDB collections have no PG-style descriptions, butmaxTablescould still be useful to limit the number of collections inspected for large databases. The function acceptsmaxTablesin its signature but never uses it—currently all collections are fetched vialistCollections().toArray()without filtering.packages/server/src/routes/tables.routes.ts (1)
68-73: MongoDB collection creation silently discards column definitions.When
dbType === "mongodb", onlybody.tableNameis forwarded; any column definitions the client sent viacreateTableSchemaare ignored. This is correct for schemaless MongoDB, but the client has no feedback that columns were disregarded. Consider returning a note in the response or documenting this behavior in the API to avoid confusion.packages/server/src/dao/mongo/schema.dao.ts (1)
5-15:convertColumndrops optionalColumnfields.The converter only maps
name,type,nullable, andisPrimaryKey, silently discardingforeignKey,description, andenumValueseven if they were present in the input. This is fine for current MongoDB column inference (which doesn't produce those fields), but consider using a spread or explicit pass-through so future additions aren't silently lost.packages/server/src/dao/mongo/query.dao.ts (2)
60-67: Parsed JSON payload is not runtime-validated againstMongoQueryPayload.
JSON.parse(query) as MongoQueryPayloadis a type-only assertion — it doesn't verify the shape at runtime. While lines 69-73 check forcollectionandoperation, other fields (filter,pipeline,document,update) are trusted blindly. Malformed values (e.g.,filterbeing a string instead of an object) will surface as cryptic MongoDB driver errors rather than clear 400 responses.Consider adding a Zod schema for
MongoQueryPayloadto validate the parsed JSON, consistent with how the rest of the codebase validates request inputs.
95-99: Aggregate pipeline is passed directly without sanitization.
$out,$merge, and$unionWithstages could write to or read from arbitrary collections. This is consistent with the raw-SQL design (PostgreSQL path also allows unrestricted DDL/DML), but worth noting for anyone auditing security — this endpoint effectively grants full database access.packages/server/src/dao/mongo/tables.dao.ts (3)
177-184: SequentialestimatedDocumentCountcalls for each collection.Each collection's count is awaited serially. For databases with many collections, this can be slow. Consider parallelizing with
Promise.all:Proposed refactor
- const results: TableInfoSchemaType[] = []; - for (const collection of collections) { - const name = collection.name; - const rowCount = await mongoDb.collection(name).estimatedDocumentCount(); - results.push({ tableName: name, rowCount }); - } - - return results; + return Promise.all( + collections.map(async (col) => ({ + tableName: col.name, + rowCount: await mongoDb.collection(col.name).estimatedDocumentCount(), + })), + );
346-346: Hard-coded 10,000 document limit for export with no streaming.All 10k documents are loaded into memory at once, normalized, then returned as a single response. For collections with large documents, this could cause significant memory pressure. Consider making the limit configurable or adding a streaming/chunked export path.
307-320: Minor TOCTOU between existence check and creation.Between
listCollections(line 315) andcreateCollection(line 319), a concurrent request could create the same collection, causingcreateCollectionto throw with a different error than the friendlyHTTPException. This is low-risk in practice, but wrappingcreateCollectionin a try/catch that catches the duplicate-collection error would make this fully robust.
…tProvider and registerCompletionItemProvider and disposing them in the effect cleanup..
…normalization and collection limits
🎯 Auto PR: stage → main
2 participants
(#107)
executeMongoQuery.addMongoRecord,updateMongoRecords,deleteMongoRecords, etc.).getMongoDatabaseSchema.getMongoTablesList,createMongoCollection, anddeleteMongoColumn.reSQL functionality.
Summary by CodeRabbit
Release Notes
New Features
Improvements