feat: Add Amazon DynamoDB database support#389
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new DynamoDB driver plugin and supporting app integration: AWS SigV4 HTTP client with credentials resolution (credentials/profile/SSO), PartiQL parsing/execution, tagged query builder, item flattening and statement generation, plugin driver with paging/schema sampling/filters, UI/connect form secure-field Keychain support, docs, assets, tests, and Xcode target. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant Plugin as DynamoDBPlugin
participant Driver as DynamoDBPluginDriver
participant Builder as DynamoDBQueryBuilder
participant Conn as DynamoDBConnection
participant AWS as AWS DynamoDB
UI->>Plugin: open connection form / create connection
Plugin->>Driver: createDriver(config)
Driver->>Conn: connect() — resolve credentials (credentials/profile/sso)
Conn->>AWS: POST (SigV4 signed) listTables / execute
AWS-->>Conn: 200 / error
UI->>Driver: executeQuery(statement)
alt Tagged browse/query
Driver->>Builder: parse tagged query
Builder-->>Driver: parsed params
Driver->>Conn: scan()/query() (SigV4 signed)
else PartiQL statement
Driver->>Conn: executeStatement(PartiQL) (SigV4 signed)
end
Conn-->>Driver: JSON response
Driver->>Driver: flatten items / infer columns
Driver-->>UI: tabular result / metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
docs/zh/databases/dynamodb.mdx-291-292 (1)
291-292:⚠️ Potential issue | 🟡 MinorCorrect the transport wording: this plugin does not use the AWS SDK.
Line 291-292 says DynamoDB uses HTTPS via AWS SDK, but this PR’s implementation is SigV4 with CommonCrypto (no external AWS SDK). Please update this sentence to avoid misleading users.
📝 Suggested change
-- **无 SSH 或 SSL 配置。** DynamoDB 默认通过 AWS SDK 使用 HTTPS。 +- **无 SSH 或 SSL 配置。** TablePro 直接通过 HTTPS 调用 DynamoDB API（SigV4 签名）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/zh/databases/dynamodb.mdx` around lines 291 - 292, Update the transport sentence to remove the claim that DynamoDB is accessed "via AWS SDK" and instead state that this plugin sends HTTPS requests signed with AWS SigV4 using CommonCrypto (no external AWS SDK is used); modify the line that currently reads "DynamoDB 默认通过 AWS SDK 使用 HTTPS" to clarify it uses HTTPS with SigV4 signing via CommonCrypto and that there is no SSH/SSL client library dependency.docs/zh/databases/dynamodb.mdx-127-153 (1)
127-153:⚠️ Potential issue | 🟡 MinorAdd language identifiers to the three plain fenced code blocks.
Line 127-153 uses bare triple-backtick blocks. Please annotate them (for example,
text) to match docs standards.📝 Suggested change
-``` +```text Name: Production DynamoDB Auth Method: Access Key + Secret Key Access Key ID: AKIAIOSFODNN7EXAMPLE Secret Access Key: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY Region: us-east-1@@
-+text
Name: Staging DynamoDB
Auth Method: AWS Profile
Profile Name: staging
Region: us-west-2@@ -``` +```text Name: Local DynamoDB Auth Method: Access Key + Secret Key Access Key ID: fakeAccessKey Secret Access Key: fakeSecretKey Region: us-east-1 Custom Endpoint: http://localhost:8000</details> As per coding guidelines: "Include language identifiers in fenced code blocks." <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/zh/databases/dynamodb.mdxaround lines 127 - 153, The three plain
fenced code blocks under the "Production DynamoDB", "AWS Profile" (Staging
DynamoDB) and "DynamoDB Local" examples lack language identifiers; update each
opening triple-backtick to include a language tag (usetext) so those blocks
read ```text to comply with the docs guideline "Include language identifiers in
fenced code blocks".</details> </blockquote></details> <details> <summary>TablePro/ViewModels/SidebarViewModel.swift-39-43 (1)</summary><blockquote> `39-43`: _⚠️ Potential issue_ | _🟡 Minor_ **Replace `NSLog` with structured `Logger` calls.** Lines 39, 43, 147, 152, and 155 use `NSLog`, which diverges from the project logging standard requiring OSLog. <details> <summary>Suggested change</summary> ```diff import Observation +import os import SwiftUI @@ struct LiveTableFetcher: TableFetcher { + private static let logger = Logger(subsystem: "com.TablePro", category: "LiveTableFetcher") @@ - guard let driver = await DatabaseManager.shared.driver(for: connectionId) else { - NSLog("[LiveTableFetcher] driver is nil for connectionId: %@", connectionId.uuidString) + guard let driver = await DatabaseManager.shared.driver(for: connectionId) else { + Self.logger.error("driver is nil for connectionId: \(connectionId.uuidString, privacy: .public)") return [] } let fetched = try await driver.fetchTables() - NSLog("[LiveTableFetcher] fetched %d tables", fetched.count) + Self.logger.debug("fetched \(fetched.count, privacy: .public) tables") @@ final class SidebarViewModel { + private static let logger = Logger(subsystem: "com.TablePro", category: "SidebarViewModel") @@ - guard tables.isEmpty else { - NSLog("[SidebarVM] onAppear: tables not empty (%d), skipping", tables.count) + guard tables.isEmpty else { + Self.logger.debug("onAppear: tables not empty (\(tables.count, privacy: .public)), skipping") return } @@ - NSLog("[SidebarVM] onAppear: driver found, loading tables") + Self.logger.debug("onAppear: driver found, loading tables") loadTables() } else { - NSLog("[SidebarVM] onAppear: driver is nil for %@", connectionId.uuidString) + Self.logger.error("onAppear: driver is nil for \(connectionId.uuidString, privacy: .public)") } } ``` </details> Per coding guidelines: "Use OSLog with Logger(subsystem: 'com.TablePro', category: 'ComponentName') for logging. Never use print()." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/ViewModels/SidebarViewModel.swift` around lines 39 - 43, Replace all NSLog calls in LiveTableFetcher (and SidebarViewModel) with structured OSLog Logger instances: create a Logger(subsystem: "com.TablePro", category: "LiveTableFetcher") and use its methods (info, error, debug) in place of NSLog at the locations that log driver nil, fetched count, and the other occurrences (the NSLog calls around driver.fetchTables and the lines noted). Use Logger.log-level methods and include the same contextual values (e.g., connectionId.uuidString, fetched.count, and any error) as parameters to the Logger call so logs follow the project's OSLog pattern. ``` </details> </blockquote></details> <details> <summary>TablePro/Resources/Localizable.xcstrings-374-383 (1)</summary><blockquote> `374-383`: _⚠️ Potential issue_ | _🟡 Minor_ **Format string marked as "new" and missing multi-language localizations.** The format string `"%@ (%@@%@)"` has an English value but is marked with `"state" : "new"`, indicating it's untranslated or pending. Additionally, Vietnamese and Chinese localizations are absent despite the PR documentation being provided in all three languages. <details> <summary>🌐 Recommended fix</summary> ```diff "%@ (%@@%@)" : { "localizations" : { "en" : { "stringUnit" : { - "state" : "new", + "state" : "translated", "value" : "%1$@ (%2$@@%3$@)" } + }, + "vi" : { + "stringUnit" : { + "state" : "translated", + "value" : "%1$@ (%2$@@%3$@)" + } + }, + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "%1$@ (%2$@@%3$@)" + } } } }, ``` Note: Format strings typically remain identical across locales unless parameter order changes are required by language grammar. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Resources/Localizable.xcstrings` around lines 374 - 383, The format string key "%@ (%@@%@)" currently has its English value "%1$@ (%2$@@%3$@)" marked as "state":"new" and lacks Vietnamese and Chinese localizations; change the "state" from "new" to absent (or "state":"translated") for the existing English entry and add "vi" and "zh" entries under "localizations" using the same format string value ("%1$@ (%2$@@%3$@)") unless a different parameter order is required by grammar—ensure the new locale blocks mirror the "en" structure (i.e., include a "stringUnit" with "value") so all three languages are present. ``` </details> </blockquote></details> <details> <summary>docs/databases/dynamodb.mdx-306-320 (1)</summary><blockquote> `306-320`: _⚠️ Potential issue_ | _🟡 Minor_ **Don't describe this plugin as if it used the AWS SDK.** Line 313 says retry/backoff comes from "the AWS SDK", and Line 320 says HTTPS is handled "through the AWS SDK". The implementation here is a custom `DynamoDBConnection` on top of `URLSession`, so these bullets currently over-promise the runtime behavior. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/databases/dynamodb.mdx` around lines 306 - 320, The docs incorrectly state behavior provided by the AWS SDK; update the text to reflect that this implementation is a custom DynamoDBConnection built on URLSession: replace the "AWS SDK" retry/backoff claim with guidance that retries/backoff are not provided automatically and must be implemented by the caller or by using a retry middleware around DynamoDBConnection, and replace the statement that HTTPS is handled "through the AWS SDK" with a note that secure transport is provided by the platform URL loading stack (URLSession) and any TLS configuration is platform-controlled. Locate and edit the phrases mentioning "the AWS SDK" and adjust the Solutions and Known Limitations sections to reference DynamoDBConnection and URLSession accordingly. ``` </details> </blockquote></details> <details> <summary>docs/vi/databases/dynamodb.mdx-257-296 (1)</summary><blockquote> `257-296`: _⚠️ Potential issue_ | _🟡 Minor_ **Keep the Vietnamese caveats aligned with the English page.** The English page also covers throughput-exceeded troubleshooting plus the missing transactions UI and Global Tables management, while this localized page stops earlier. Users currently get different caveats depending on language. As per coding guidelines "Update both English (docs/) and Vietnamese (docs/vi/) pages." <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/vi/databases/dynamodb.mdx` around lines 257 - 296, The Vietnamese DynamoDB troubleshooting/caveats page is missing items present on the English page; update docs/vi/databases/dynamodb.mdx to mirror the English page by adding the "throughput exceeded" troubleshooting section, notes about the missing Transactions UI, and guidance on Global Tables management, and ensure the list of caveats matches the English docs exactly; edit the relevant headings and bullet lists in docs/vi/databases/dynamodb.mdx (the "Xử lý Sự cố" and "Hạn chế" sections) so content parity with docs/databases/dynamodb.mdx is achieved and follow the project guideline to keep both docs/ and docs/vi/ synchronized. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>TablePro/Models/Connection/DatabaseConnection.swift (1)</summary><blockquote> `240-240`: **Add a parity test for the new `DatabaseType.dynamodb` raw value.** Other database type constants have direct rawValue tests; adding one for DynamoDB will prevent accidental regressions in type-id matching. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Models/Connection/DatabaseConnection.swift` at line 240, Add a unit test asserting the new DatabaseType.dynamodb raw value equals "DynamoDB" to match existing parity tests for other database types; locate the tests that check other DatabaseType constants (e.g., where DatabaseType.sqlite.rawValue or DatabaseType.postgres.rawValue are asserted) and add a similar assertion for DatabaseType.dynamodb.rawValue == "DynamoDB" so the enum's raw value cannot regress. ``` </details> </blockquote></details> <details> <summary>TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift (1)</summary><blockquote> `1143-1160`: **Make `awsRegion` editable.** A closed dropdown means every AWS region addition needs a new app release, and there is no fallback if a user's region is not in this list. A text field, or at least an editable picker, with `us-east-1` as the default would age better. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Plugins/PluginMetadataRegistry`+RegistryDefaults.swift around lines 1143 - 1160, The awsRegion metadata entry (id: "awsRegion") currently uses a closed dropdown (fieldType) with hard-coded options; change it to an editable input so users can enter any region while keeping defaultValue: "us-east-1". Locate the awsRegion definition and replace the closed dropdown fieldType with an editable control (e.g., switch to .text fieldType or an editable dropdown variant supported by the metadata API), preserve defaultValue, and keep or adjust the label; ensure any existing consumers of the field read free-form strings and update validation/parsing where getPlugin metadata is used to accept arbitrary region strings. ``` </details> </blockquote></details> <details> <summary>docs/databases/dynamodb.mdx (1)</summary><blockquote> `127-153`: **Add language identifiers to these example fences.** The three configuration snippets render as generic code blocks today. `text` would be enough here. As per coding guidelines "Include language identifiers in fenced code blocks." <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/databases/dynamodb.mdxaround lines 127 - 153, The three fenced
examples ("Production DynamoDB", "AWS Profile", "DynamoDB Local") are missing
language identifiers; update each triple-backtick fence to include a language
tag (use "text") so the blocks becometext andtext respectively, ensuring
all three code fences for those named examples include the language identifier.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift:
- Around line 403-420: The created URLSessionDataTask inside the
withCheckedThrowingContinuation isn't stored into the connection's _currentTask,
so cancelCurrentRequest() and disconnect() can't cancel it; assign the new task
to _currentTask immediately after creating it (before task.resume()) and ensure
you clear _currentTask (set to nil) when the completion handler resumes the
continuation (both success and failure paths). Reference the
urlSession.dataTask(...) task, the _currentTask property, and the
cancelCurrentRequest()/disconnect() callers and update the lifecycle handling to
set/clear _currentTask around task.resume().- Around line 258-262: The current endpoint selection in
DynamoDBConnection.swift (where endpointUrl is set from
config.additionalFields["awsEndpointUrl"]) must validate and restrict insecure
(http) endpoints: parse the provided URL string, and only accept http scheme if
the host is loopback/localhost (e.g., "localhost", "127.0.0.1", "::1") or in a
loopback range; otherwise require https and reject or ignore plain http hosts
unless an explicit opt-in flag is present (e.g.,
config.additionalFields["allowInsecureEndpoint"] == "true"). Update the logic
around endpointUrl assignment to perform this URL parsing and checks, and
log/throw a clear error when an insecure non-local endpoint is provided without
the opt-in override.- Around line 442-448: The decode-failure handler and credential-logging paths
currently emit raw response bodies and credential details; update them to never
log sensitive content and to use OSLog Logger(subsystem: "com.TablePro",
category: "DynamoDBConnection") instead of free-form messages. In the JSON
decode catch block (where Self.logger.error is called) remove the raw body
output and replace it with metadata-only diagnostics (e.g., target identifier,
response length or a fixed "" marker and the decode error message).
Similarly, find the credential resolver/logging code that prints access key IDs
and auth-field metadata and redact those fields (log only that credentials were
resolved and non-sensitive metadata like "credentialSource: X" or
""). Ensure all logging uses Logger(subsystem:
"com.TablePro", category: "DynamoDBConnection") and apply the same redaction
change to the other occurrence noted around lines 578-580.- Around line 614-623: The parsing currently uses
trimmed.components(separatedBy: "=") which breaks values that contain '=' (e.g.,
aws_session_token); update the loop in DynamoDBConnection.swift to split on the
first '=' only: find the first '=' index (e.g., using trimmed.firstIndex(of:
"=")), obtain the key as the substring before that index and the value as the
substring after that index, trim both, and then use the switch on the key to
assign accessKeyId, secretAccessKey, and sessionToken (replacing the current
parts-based logic and guard).- Around line 640-676: The resolveSsoCredentials() function is reading the wrong
cache location and JSON fields; change the scanned directory from
"/.aws/sso/cache" to "/.aws/cli/cache" (update the ssoCachePath variable name
to cliCachePath or similar), and parse the CLI cache JSON entries for AWS role
credentials (look for "AccessKeyId", "SecretAccessKey", "SessionToken" and the
expiration field, e.g., "Expiration" or a top-level timestamp) instead of the
SSO/OIDC fields; keep the same logic that selects the newest non-expired token
into latestToken and update the thrown DynamoDBError.authFailed message to
reference ~/.aws/cli/cache and suggest running 'aws sso login' if none found.In
@Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift:
- Around line 115-135: The awsRegion field currently hardcodes a limited
dropdown list (fieldType: .dropdown with options) which excludes many valid AWS
regions; change the UI to either (A) use a free-form text input (change
fieldType from .dropdown to a text input) while keeping defaultValue "us-east-1"
and validating input against known region pattern, or (B) populate the dropdown
dynamically from AWS’s maintained region list at runtime (use AWS SDK/endpoint
list or a maintained JSON) so options for awsRegion are always up-to-date;
update any validation or usage code referencing awsRegion to accept the
free-form value or the dynamically fetched options accordingly.In
@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift:
- Around line 917-925: The current code injects a synthetic row ("No items
returned") into the PluginQueryResult when response.Items is empty; instead,
change the branch that handles items.isEmpty to return an empty rows array
(rows: []) and rowsAffected: 0 while still returning the appropriate
columns/columnTypeNames and executionTime so downstream callers like
fetchRows/export/copy see zero real rows rather than a placeholder; update the
PluginQueryResult construction where items, columns, columnTypeNames,
rowsAffected, and executionTime are set.- Around line 139-149: The current branches calling
DynamoDBQueryBuilder.parseCountQuery, parseScanQuery, and parseQueryQuery
discard parsed filter/key details by calling countItems(tableName:conn:) which
returns the full-table total; instead, extract and forward the parsed filter
expression, key condition, and any expression attribute names/values from the
parsed result into a counting operation that performs a filtered Scan or Query
(e.g., a count-only DynamoDB Query/Scan using Select: "COUNT" or a countItems
overload that accepts filter/keyCondition + expression attributes). Update the
branches that use parseCountQuery, parseScanQuery, and parseQueryQuery to call
the appropriate counting helper (or add a new
countItems(tableName:conn:filter:keyCondition:exprNames:exprValues:)) and pass
the parsed filter/key information so the returned count reflects only matching
items.- Around line 528-533: The code is pulling column types from an arbitrary
pagination state which can belong to a different table; change the lookup to use
the pagination state keyed by the current table instead of
_paginationStates.values.first(...). Inside the lock.withLock block, retrieve
the state via _paginationStates[table] (or the correct table key variable used
in this scope) and use its columnTypeNames if present, otherwise fall back to
columns.map { _ in "S" }; ensure this updated typeNames is what you pass into
DynamoDBStatementGenerator so type hints come from the matching table's
columnTypeNames.In
@Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift:
- Around line 67-70: The current QUERY path only preserves the HASH-key equality
and the SCAN path encodes only filters.first, which drops other predicates and
the logicMode; update the logic in DynamoDBQueryBuilder.swift (around
partitionKey, pkFilter, filters usage) to: detect and remove the pk equality
from the general filters but retain all remaining filters; for QUERY build a
KeyConditionExpression from the partition key equality (if present) and
construct a FilterExpression that combines all other predicates using the
request's logicMode (AND/OR) and proper operator mapping; for SCAN build the
FilterExpression by combining all filters (not just the first) using logicMode;
ensure helper code that maps Filter.op to DynamoDB operators and parameter
placeholders is reused for both QUERY and SCAN so no predicate or logicMode is
lost.- Around line 118-128: Current code returns buildQuickSearchQuery whenever
searchText is non-empty, which ignores filters and logicMode; change this to
preserve structured filters by combining them with the quick-search criteria:
either update buildQuickSearchQuery (or create a new helper) to accept filters
and logicMode and merge those predicates with the generated quick-search
predicate, or call buildFilteredQuery and pass the quick-search predicate into
its filters parameter so both searchText and filters are applied. Locate the
early-return block around searchText and modify the call to use the
extended/combined function (referencing buildQuickSearchQuery,
buildFilteredQuery, searchText, filters, and logicMode) so the returned query
applies both the quick-search and the structured filters together.- Around line 71-79: The code currently hard-codes pkType = "S" before calling
Self.encodeQueryQuery which breaks non-string partition keys; instead, read the
actual attribute type for the partition key (the same source used in
DynamoDBConnection's TableDescription.AttributeDefinitions) and map it to the
DynamoDB tag (e.g., "S","N","B"), then pass that mapped value as pkType into
encodeQueryQuery; update the code that constructs pkType (and any helper mapping
function if needed) so encodeQueryQuery receives the real partitionKeyType
rather than the fixed "S".In
@Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift:
- Around line 170-180: The formatValue(_:, typeName:) implementation silently
coerces invalid values for numeric ("N") and boolean ("BOOL") types; update it
to validate strictly instead of converting: for case "N" attempt to parse as
Int64 or Double and if parsing fails surface a validation error (throw or return
a failure) rather than returning a quoted string, and for case "BOOL" accept
only explicit true tokens ("true","1") or false tokens ("false","0") and surface
a validation error for any other input instead of defaulting to false; update
the function signature (e.g., make formatValue throw or return an
optional/result) and adjust callers to handle the validation error accordingly
so invalid typed edits do not get silently coerced.In
@TablePro.xcodeproj/project.pbxproj:
- Around line 3149-3166: Add LD_RUNPATH_SEARCH_PATHS to both build
configurations for the DynamoDBDriverPlugin target so the
TableProPluginKit.framework can be resolved at runtime; specifically, in the
target block that identifies the DynamoDBDriverPlugin (e.g.,
PRODUCT_BUNDLE_IDENTIFIER = com.TablePro.DynamoDBDriverPlugin; or
INFOPLIST_KEY_NSPrincipalClass = "$(PRODUCT_MODULE_NAME).DynamoDBPlugin") add
LD_RUNPATH_SEARCH_PATHS = "$(inherited)@loader_path/../Frameworks"; for each
configuration alongside the existing keys (like SWIFT_VERSION,
WRAPPER_EXTENSION, SKIP_INSTALL) so the plugin will find
TableProPluginKit.framework when loaded.In
@TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift:
- Around line 1122-1134: The awsSecretAccessKey and awsSessionToken
ConnectionField entries are being treated as UI-only secure fields but are saved
in existing.additionalFields (see ConnectionFormView.loadConnectionData and
saveConnection), which persists them with the connection record; change these
fields to use the same encrypted Keychain storage flow used by the password/SSH
secrets instead: remove them from additionalFields, add Keychain-backed keys (or
reuse the existing secret storage helper used for "password"/SSH), update
ConnectionFormView.loadConnectionData() to pull these values from the Keychain
when populating the form and update saveConnection() to write them to the
Keychain (and not into additionalFields), and ensure any migration moves any
existing values from additionalFields into the Keychain.In
@TablePro/Resources/Localizable.xcstrings:
- Around line 915-917: Several new localization entries in Localizable.xcstrings
were added with empty blocks, causing String(localized:) lookups (used in
ConnectionFormView.swift, DynamoDBPlugin.swift,
PluginMetadataRegistry+RegistryDefaults.swift) to resolve to empty strings;
populate each of the 25 affected keys (examples: "Delete SSH Profile?", "This
profile will be permanently deleted.", "Selected SSH profile no longer exists.",
"Access Key ID", "Secret Access Key", "Auth Method", "Profile Name", "AWS
Region", "Custom Endpoint", "SSH Profile", "SSH Profiles:", "Inline
Configuration", and the other added keys) with a proper localization structure
by adding a localizations > en > stringUnit entry that sets state and value
(value can be the English key text), and optionally add vi/zh translations per
the PR scope so runtime String(localized:) returns the expected text.In
@TablePro/Views/Connection/ConnectionFormView.swift:
- Around line 970-983: The isValid computed property currently skips secret
checks when hidePasswordField is true, which lets forms like DynamoDB (default
awsAuthMethod == "credentials") pass without awsAccessKeyId/awsSecretAccessKey;
update isValid (in ConnectionFormView) to also inspect auth-specific fields and
require credentials when the selected auth method needs them (e.g., check
additionalFieldValues["awsAuthMethod"] or the auth field id for auth method and
when it equals "credentials" ensure additionalFieldValues contains non-empty
awsAccessKeyId and awsSecretAccessKey), or use authSectionFields metadata
(isRequired) keyed by auth method to enforce required secret fields even if
hidePasswordField is true, so Save/Test remain disabled until required AWS
credentials are present.
Minor comments:
In@docs/databases/dynamodb.mdx:
- Around line 306-320: The docs incorrectly state behavior provided by the AWS
SDK; update the text to reflect that this implementation is a custom
DynamoDBConnection built on URLSession: replace the "AWS SDK" retry/backoff
claim with guidance that retries/backoff are not provided automatically and must
be implemented by the caller or by using a retry middleware around
DynamoDBConnection, and replace the statement that HTTPS is handled "through the
AWS SDK" with a note that secure transport is provided by the platform URL
loading stack (URLSession) and any TLS configuration is platform-controlled.
Locate and edit the phrases mentioning "the AWS SDK" and adjust the Solutions
and Known Limitations sections to reference DynamoDBConnection and URLSession
accordingly.In
@docs/vi/databases/dynamodb.mdx:
- Around line 257-296: The Vietnamese DynamoDB troubleshooting/caveats page is
missing items present on the English page; update docs/vi/databases/dynamodb.mdx
to mirror the English page by adding the "throughput exceeded" troubleshooting
section, notes about the missing Transactions UI, and guidance on Global Tables
management, and ensure the list of caveats matches the English docs exactly;
edit the relevant headings and bullet lists in docs/vi/databases/dynamodb.mdx
(the "Xử lý Sự cố" and "Hạn chế" sections) so content parity with
docs/databases/dynamodb.mdx is achieved and follow the project guideline to keep
both docs/ and docs/vi/ synchronized.In
@docs/zh/databases/dynamodb.mdx:
- Around line 291-292: Update the transport sentence to remove the claim that
DynamoDB is accessed "via AWS SDK" and instead state that this plugin sends
HTTPS requests signed with AWS SigV4 using CommonCrypto (no external AWS SDK is
used); modify the line that currently reads "DynamoDB 默认通过 AWS SDK 使用 HTTPS" to
clarify it uses HTTPS with SigV4 signing via CommonCrypto and that there is no
SSH/SSL client library dependency.- Around line 127-153: The three plain fenced code blocks under the "Production
DynamoDB", "AWS Profile" (Staging DynamoDB) and "DynamoDB Local" examples lack
language identifiers; update each opening triple-backtick to include a language
tag (usetext) so those blocks read ```text to comply with the docs guideline
"Include language identifiers in fenced code blocks".In
@TablePro/Resources/Localizable.xcstrings:
- Around line 374-383: The format string key "%@ (%@@%@)" currently has its
English value "%1$@ (%2$@@%3$@)" marked as "state":"new" and lacks Vietnamese
and Chinese localizations; change the "state" from "new" to absent (or
"state":"translated") for the existing English entry and add "vi" and "zh"
entries under "localizations" using the same format string value ("%1$@
(%2$@@%3$@)") unless a different parameter order is required by grammar—ensure
the new locale blocks mirror the "en" structure (i.e., include a "stringUnit"
with "value") so all three languages are present.In
@TablePro/ViewModels/SidebarViewModel.swift:
- Around line 39-43: Replace all NSLog calls in LiveTableFetcher (and
SidebarViewModel) with structured OSLog Logger instances: create a
Logger(subsystem: "com.TablePro", category: "LiveTableFetcher") and use its
methods (info, error, debug) in place of NSLog at the locations that log driver
nil, fetched count, and the other occurrences (the NSLog calls around
driver.fetchTables and the lines noted). Use Logger.log-level methods and
include the same contextual values (e.g., connectionId.uuidString,
fetched.count, and any error) as parameters to the Logger call so logs follow
the project's OSLog pattern.
Nitpick comments:
In@docs/databases/dynamodb.mdx:
- Around line 127-153: The three fenced examples ("Production DynamoDB", "AWS
Profile", "DynamoDB Local") are missing language identifiers; update each
triple-backtick fence to include a language tag (use "text") so the blocks
becometext andtext respectively, ensuring all three code fences for
those named examples include the language identifier.In
@TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift:
- Around line 1143-1160: The awsRegion metadata entry (id: "awsRegion")
currently uses a closed dropdown (fieldType) with hard-coded options; change it
to an editable input so users can enter any region while keeping defaultValue:
"us-east-1". Locate the awsRegion definition and replace the closed dropdown
fieldType with an editable control (e.g., switch to .text fieldType or an
editable dropdown variant supported by the metadata API), preserve defaultValue,
and keep or adjust the label; ensure any existing consumers of the field read
free-form strings and update validation/parsing where getPlugin metadata is used
to accept arbitrary region strings.In
@TablePro/Models/Connection/DatabaseConnection.swift:
- Line 240: Add a unit test asserting the new DatabaseType.dynamodb raw value
equals "DynamoDB" to match existing parity tests for other database types;
locate the tests that check other DatabaseType constants (e.g., where
DatabaseType.sqlite.rawValue or DatabaseType.postgres.rawValue are asserted) and
add a similar assertion for DatabaseType.dynamodb.rawValue == "DynamoDB" so the
enum's raw value cannot regress.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `83a4b32e-82c1-40a6-84cc-4cc05f2fc3c7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1aa2ebf9fd56e74c78d6c2573d20bc652e60c9ec and 69019d07e76725ca37442f7620a14cd0b5fa544f. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `TablePro/Assets.xcassets/dynamodb-icon.imageset/dynamodb.svg` is excluded by `!**/*.svg` </details> <details> <summary>📒 Files selected for processing (21)</summary> * `CHANGELOG.md` * `Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBItemFlattener.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift` * `Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift` * `Plugins/DynamoDBDriverPlugin/Info.plist` * `TablePro.xcodeproj/project.pbxproj` * `TablePro/Assets.xcassets/dynamodb-icon.imageset/Contents.json` * `TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift` * `TablePro/Models/Connection/DatabaseConnection.swift` * `TablePro/Resources/Localizable.xcstrings` * `TablePro/ViewModels/SidebarViewModel.swift` * `TablePro/Views/Connection/ConnectionFormView.swift` * `TablePro/Views/Main/MainContentCoordinator.swift` * `docs/databases/dynamodb.mdx` * `docs/docs.json` * `docs/vi/databases/dynamodb.mdx` * `docs/zh/databases/dynamodb.mdx` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Implements a new DynamoDB driver plugin with PartiQL query support, AWS IAM/Profile/SSO authentication, GSI/LSI browsing, table scanning with cursor-based pagination, capacity/metrics display, DynamoDB Local support, and cell editing via PartiQL INSERT/UPDATE/DELETE generation.
datlechin
force-pushed
the
feat/dynamodb-support
branch
from
69019d0 to
950c403
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift (1)
1121-1137:⚠️ Potential issue | 🔴 CriticalDo not persist AWS secrets via
additionalFields.The TODO is accurate: these
.securefields are still loaded from and saved back toDatabaseConnection.additionalFields, soawsSecretAccessKeyandawsSessionTokenremain in the connection record instead of the Keychain flow used forpassword. This is a release blocker for credential handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Core/Plugins/PluginMetadataRegistry`+RegistryDefaults.swift around lines 1121 - 1137, The awsSecretAccessKey and awsSessionToken ConnectionField entries are marked .secure but still read from/written to DatabaseConnection.additionalFields; migrate these fields to the Keychain flow via ConnectionStorage: update the connection save/load logic to store/retrieve awsSecretAccessKey and awsSessionToken from the Keychain (using the same mechanism as password), remove them from DatabaseConnection.additionalFields usage, and add a one-time migration in ConnectionStorage that, when encountering awsSecretAccessKey/awsSessionToken in additionalFields, moves their values into the Keychain and deletes those keys from additionalFields; update any code that references these IDs (awsSecretAccessKey, awsSessionToken, ConnectionField, DatabaseConnection.additionalFields, ConnectionStorage) to use the Keychain-backed API.Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift (2)
547-559:⚠️ Potential issue | 🟠 MajorDo not pick an arbitrary cached state for this table.
This still walks
_paginationStates.valuesand returns the first state whose parsed table name matches. If the same table has multiple browse/filter queries cached, dictionary iteration order makes the chosencolumnTypeNamesnondeterministic, so edit generation can serialize values with the wrong DynamoDB type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift` around lines 547 - 559, The code currently scans _paginationStates.values and picks the first state whose parsed table matches, which is nondeterministic when a table has multiple cached queries; instead, locate the pagination state by the exact query key for this request (e.g. use _paginationStates[queryKey] or filter for state.queryKey == queryKey inside lock.withLock) and only use that state's columnTypeNames; if no exact match exists, fall back to a safe default rather than the first matching table-level state. Ensure you reference and use the same queryKey used to build the DynamoDBQueryBuilder parse calls and return its state.columnTypeNames.
151-165:⚠️ Potential issue | 🟠 MajorFiltered count queries are still not using the full filter set.
parseScanQueryonly counts againstfilters.first,countQueryItemsdropsparsed.filtersentirely, and the tagged count execution path falls back to full-tablecountItems. Count badges and pagination will still drift from the filtered result set.Also applies to: 600-601, 1130-1149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift` around lines 151 - 165, The count code is only applying the first filter or dropping filters entirely—update the flow so all parsed.filters are used: modify the call sites that currently call countFilteredScanItems(tableName:parsed.tableName, filterColumn:..., filterOp:..., filterValue:...) and countItems(tableName:parsed.tableName) to instead pass the full parsed.filters array (from DynamoDBQueryBuilder.parseScanQuery and DynamoDBQueryBuilder.parseQueryQuery) into a new/updated counting helper (e.g., extend countFilteredScanItems and countQueryItems to accept [Filter] or similar and apply every filter when building the DynamoDB FilterExpression/KeyConditionExpression), and ensure the tagged count execution path no longer falls back to countItems but routes through the filtered-count helper so pagination and badges reflect the filtered result set.
🧹 Nitpick comments (2)
Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift (2)
247-289: Consider documenting the minimum part count rationale.The check
parts.count >= 6on line 251 allows for optional filters and logicMode fields (parts 7 and 8), which is good for compatibility. However, the encoding always produces 8 parts. A brief comment would clarify this is intentional for backward compatibility.📝 Optional: Add clarifying comment
static func parseQueryQuery(_ query: String) -> DynamoDBParsedQueryQuery? { guard query.hasPrefix(queryTag) else { return nil } let body = String(query.dropFirst(queryTag.count)) let parts = body.components(separatedBy: ":") + // Minimum 6 parts required (table, limit, offset, pkName, pkValue, pkType) + // Parts 7 (filters) and 8 (logicMode) are optional for backward compatibility guard parts.count >= 6 else { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift` around lines 247 - 289, Add a short clarifying comment in parseQueryQuery explaining why the guard uses parts.count >= 6 (allowing optional filters and logicMode), noting that the encoder currently emits 8 parts but the lower bound is kept for backward compatibility; place this comment near the guard checking parts.count >= 6 so future readers of DynamoDBQueryBuilder.swift see the rationale immediately when inspecting the parts parsing logic.
13-49: Add explicit access control to all types.Per coding guidelines, all types must have explicit access control. The structs
DynamoDBFilterSpec,DynamoDBParsedScanQuery,DynamoDBParsedQueryQuery,DynamoDBParsedCountQuery, andDynamoDBQueryBuilderare missing access modifiers.♻️ Proposed fix to add access control
-struct DynamoDBFilterSpec: Codable { +internal struct DynamoDBFilterSpec: Codable { let column: String let op: String let value: String } // MARK: - Parsed Query Types -struct DynamoDBParsedScanQuery { +internal struct DynamoDBParsedScanQuery { let tableName: String ... } -struct DynamoDBParsedQueryQuery { +internal struct DynamoDBParsedQueryQuery { let tableName: String ... } -struct DynamoDBParsedCountQuery { +internal struct DynamoDBParsedCountQuery { let tableName: String ... } // MARK: - Query Builder -struct DynamoDBQueryBuilder { +internal struct DynamoDBQueryBuilder {As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift` around lines 13 - 49, Add explicit access control keywords to all top-level types so they no longer rely on default access; update the declarations for DynamoDBFilterSpec, DynamoDBParsedScanQuery, DynamoDBParsedQueryQuery, DynamoDBParsedCountQuery, and DynamoDBQueryBuilder to include the intended access level (e.g., internal or public) before each struct name and keep existing conformances (like Codable) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/databases/dynamodb.mdx`:
- Around line 71-85: Update the AWS SSO docs to point to the actual credential
cache path used by the implementation: replace references to "~/.aws/sso/cache"
with "~/.aws/cli/cache" in the "AWS SSO" section (including the prose and any
example lines like `aws sso login` and the Warning block) so readers are
directed to the correct SSO role credential cache location used by the code.
- Around line 312-320: Replace inaccurate AWS SDK statements with details about
this plugin's actual implementation: state that DynamoDBConnection uses
URLSession for HTTP transport and a custom SigV4 signer (mention
DynamoDBConnection and SigV4) rather than the AWS SDK; remove claims like "the
AWS SDK includes automatic retry with backoff" and "HTTPS by default through the
AWS SDK" and instead document the plugin's real behavior (e.g., URLSession is
used for HTTPS transport, any TLS behavior is governed by URLSession/system
trust, and there is no implicit SDK retry/backoff unless implemented in
DynamoDBConnection), and add guidance to callers to implement retry with
exponential backoff or point to the module/location where retries would be
implemented if desired.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBItemFlattener.swift`:
- Around line 90-99: attributeValueToString() currently serializes .list/.map
using listToJson()/mapToJson() which outputs plain JSON values, but
stringToAttributeValue() expects DynamoDB-typed envelopes (e.g. [{"S":"x"}] or
{"k":{"S":"v"}}) so edits lose the original L/M types; update the .list and .map
branches to serialize each element/value as a DynamoDBAttributeValue envelope
(use the same helper that builds attribute-value JSON envelopes or recursively
call the routine that converts an AttributeValue into its typed JSON form) so
that attributeValueToString() emits the typed representation that
stringToAttributeValue() can parse back into .list/.map (also apply the same
envelope-style serialization fix for the similar cases around lines 123-136).
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swift`:
- Around line 46-81: extractTableName is returning identifiers with trailing
statement terminators (e.g. "\"Users\";") because tokenize keeps the semicolon
attached; update extractTableName (and its callers that pick tokens: the
SELECT/FROM, INSERT INTO, UPDATE, DELETE FROM branches) to post-process the
candidate token by calling unquoteIdentifier and then stripping any trailing
semicolon (;) and other common terminators (comma, closing parenthesis) and
whitespace before returning the table name so executePartiQL/describe/count use
the real table name.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift`:
- Around line 231-238: The NSLog calls and detailed error logs in fetchTables
are leaking schema/query metadata and bypass the project Logger; remove the
NSLog(...) lines and stop logging raw table names—use Self.logger (the Logger
created for this component) and log only aggregate or redacted info (e.g., count
or masked names) and include the error via error.localizedDescription where
appropriate; update the fetchTables return-area logging replacing NSLog with
Self.logger.debug/info that omits full table name lists, and apply the same
change to the other occurrences that use NSLog/print-style logging (the spots
around the other NSLog usages mentioned) to ensure all logging uses the project
Logger and does not expose raw schema or query strings.
- Around line 679-700: The loop wrongly treats a response with fewer items than
requested as end-of-data; update the pagination so it only stops because
LastEvaluatedKey is nil (or because we've reached fetchLimit), not because
items.count < batchLimit. Specifically, in the scan/query loops (look for
variables/functions: parsed.limit, parsed.offset, fetchLimit, batchLimit,
allItems, lastEvaluatedKey, conn.scan, and applyClientFilters) remove the
conditional that breaks when items.count < batchLimit and instead rely on
lastEvaluatedKey to continue paging; keep the check that exits once
allItems.count >= fetchLimit to honor client limits, and ensure the loop
condition uses lastEvaluatedKey (e.g., while lastEvaluatedKey != nil &&
allItems.count < fetchLimit) so client-side filtering and 1 MB truncated pages
do not prematurely stop pagination.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift`:
- Around line 179-205: formatValue currently mishandles Set/Binary types and
always returns "NULL" which can overwrite edits; update the formatValue(_:,
typeName:) function to: add explicit cases for "SS" and "NS" that parse the
incoming value as a list/array and emit PartiQL set literals using <<'a','b'>>
for strings (escape via escapePartiQL) and <<1,2>> for numbers (validate with
Int64/Double like the "N" case), add a "B" and "BS" branch that does not attempt
to synthesize a PartiQL literal but instead throws a clear error (or returns a
sentinel) instructing callers to use parameter binding/AttributeValue for binary
data, and change the "NULL" case to only return "NULL" when the incoming value
explicitly represents null (e.g., exact "NULL" or a mapped null token) otherwise
fall through to normal string/JSON handling; reference formatValue(_:typeName:),
escapePartiQL(value), and DynamoDBStatementError for validation/error cases when
implementing these branches.
---
Duplicate comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift`:
- Around line 547-559: The code currently scans _paginationStates.values and
picks the first state whose parsed table matches, which is nondeterministic when
a table has multiple cached queries; instead, locate the pagination state by the
exact query key for this request (e.g. use _paginationStates[queryKey] or filter
for state.queryKey == queryKey inside lock.withLock) and only use that state's
columnTypeNames; if no exact match exists, fall back to a safe default rather
than the first matching table-level state. Ensure you reference and use the same
queryKey used to build the DynamoDBQueryBuilder parse calls and return its
state.columnTypeNames.
- Around line 151-165: The count code is only applying the first filter or
dropping filters entirely—update the flow so all parsed.filters are used: modify
the call sites that currently call
countFilteredScanItems(tableName:parsed.tableName, filterColumn:...,
filterOp:..., filterValue:...) and countItems(tableName:parsed.tableName) to
instead pass the full parsed.filters array (from
DynamoDBQueryBuilder.parseScanQuery and DynamoDBQueryBuilder.parseQueryQuery)
into a new/updated counting helper (e.g., extend countFilteredScanItems and
countQueryItems to accept [Filter] or similar and apply every filter when
building the DynamoDB FilterExpression/KeyConditionExpression), and ensure the
tagged count execution path no longer falls back to countItems but routes
through the filtered-count helper so pagination and badges reflect the filtered
result set.
In `@TablePro/Core/Plugins/PluginMetadataRegistry`+RegistryDefaults.swift:
- Around line 1121-1137: The awsSecretAccessKey and awsSessionToken
ConnectionField entries are marked .secure but still read from/written to
DatabaseConnection.additionalFields; migrate these fields to the Keychain flow
via ConnectionStorage: update the connection save/load logic to store/retrieve
awsSecretAccessKey and awsSessionToken from the Keychain (using the same
mechanism as password), remove them from DatabaseConnection.additionalFields
usage, and add a one-time migration in ConnectionStorage that, when encountering
awsSecretAccessKey/awsSessionToken in additionalFields, moves their values into
the Keychain and deletes those keys from additionalFields; update any code that
references these IDs (awsSecretAccessKey, awsSessionToken, ConnectionField,
DatabaseConnection.additionalFields, ConnectionStorage) to use the
Keychain-backed API.
---
Nitpick comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swift`:
- Around line 247-289: Add a short clarifying comment in parseQueryQuery
explaining why the guard uses parts.count >= 6 (allowing optional filters and
logicMode), noting that the encoder currently emits 8 parts but the lower bound
is kept for backward compatibility; place this comment near the guard checking
parts.count >= 6 so future readers of DynamoDBQueryBuilder.swift see the
rationale immediately when inspecting the parts parsing logic.
- Around line 13-49: Add explicit access control keywords to all top-level types
so they no longer rely on default access; update the declarations for
DynamoDBFilterSpec, DynamoDBParsedScanQuery, DynamoDBParsedQueryQuery,
DynamoDBParsedCountQuery, and DynamoDBQueryBuilder to include the intended
access level (e.g., internal or public) before each struct name and keep
existing conformances (like Codable) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb292890-c30e-445c-8370-a1abf2a99354
⛔ Files ignored due to path filters (1)
TablePro/Assets.xcassets/dynamodb-icon.imageset/dynamodb.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
CHANGELOG.mdPlugins/DynamoDBDriverPlugin/DynamoDBConnection.swiftPlugins/DynamoDBDriverPlugin/DynamoDBItemFlattener.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPlugin.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swiftPlugins/DynamoDBDriverPlugin/DynamoDBQueryBuilder.swiftPlugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swiftPlugins/DynamoDBDriverPlugin/Info.plistTablePro.xcodeproj/project.pbxprojTablePro.xcodeproj/xcshareddata/xcschemes/TablePro.xcschemeTablePro/Assets.xcassets/dynamodb-icon.imageset/Contents.jsonTablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swiftTablePro/Models/Connection/DatabaseConnection.swiftTablePro/Resources/Localizable.xcstringsTablePro/ViewModels/SidebarViewModel.swiftTablePro/Views/Connection/ConnectionFormView.swiftTablePro/Views/Main/MainContentCoordinator.swiftdocs/databases/dynamodb.mdxdocs/docs.jsondocs/vi/databases/dynamodb.mdxdocs/zh/databases/dynamodb.mdx
✅ Files skipped from review due to trivial changes (7)
- TablePro.xcodeproj/xcshareddata/xcschemes/TablePro.xcscheme
- Plugins/DynamoDBDriverPlugin/Info.plist
- TablePro/Assets.xcassets/dynamodb-icon.imageset/Contents.json
- CHANGELOG.md
- TablePro/Models/Connection/DatabaseConnection.swift
- docs/docs.json
- docs/vi/databases/dynamodb.mdx
🚧 Files skipped from review as they are similar to previous changes (5)
- TablePro/Views/Main/MainContentCoordinator.swift
- TablePro/ViewModels/SidebarViewModel.swift
- docs/zh/databases/dynamodb.mdx
- TablePro.xcodeproj/project.pbxproj
- Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift
There was a problem hiding this comment.
🧹 Nitpick comments (5)
TableProTests/Plugins/DynamoDBQueryBuilderTests.swift (2)
11-31: Add explicit access control to test structs.Per coding guidelines, types should have explicit access control. While test structs default to
internal, specifying it explicitly improves clarity.Suggested change
`@Suite`("DynamoDBQueryBuilder - Browse Query") -struct DynamoDBQueryBuilderBrowseTests { +internal struct DynamoDBQueryBuilderBrowseTests { private let builder = DynamoDBQueryBuilder()Apply the same pattern to all test structs in this file.
As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Plugins/DynamoDBQueryBuilderTests.swift` around lines 11 - 31, The test struct DynamoDBQueryBuilderBrowseTests (and any other test structs in this file) must declare explicit access control instead of relying on the default; update the struct declaration for DynamoDBQueryBuilderBrowseTests to include an access modifier (e.g., internal) and do the same for any other test structs so types like DynamoDBQueryBuilderBrowseTests and its test methods browseReturnsScanTag and browseRoundTrip have explicit visibility per the coding guideline.
50-52: Avoid force unwrapping in tests — use#expectwith optional binding instead.Force unwrapping (
query!) causes test crashes rather than graceful failures when the value is unexpectedly nil. Consider usingtry#require()or guard statements for clearer failure messages.Suggested approach
- `#expect`(query != nil) - `#expect`(query!.hasPrefix(DynamoDBQueryBuilder.scanTag)) + let unwrappedQuery = try `#require`(query) + `#expect`(unwrappedQuery.hasPrefix(DynamoDBQueryBuilder.scanTag))Apply similar changes to other force-unwrap sites (lines 68, 88-93, 113-115, 172, 190, 210, 228, 277, 353).
As per coding guidelines: "Never use force unwrapping or force casting. Use guard let, if let, or as? instead."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Plugins/DynamoDBQueryBuilderTests.swift` around lines 50 - 52, Tests currently force-unwrap the optional `query` (e.g., `query!`) which can crash; update the assertions in the failing test(s) to safely unwrap and assert presence before checking contents—use `try `#require`(query)` or an `if let`/`guard let` pattern combined with `#expect` to assert non-nil, then assert `query.hasPrefix(DynamoDBQueryBuilder.scanTag)` (and similarly replace other force-unwrap sites referenced: lines 68, 88-93, 113-115, 172, 190, 210, 228, 277, 353) so tests fail gracefully with clear messages rather than crashing.Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift (2)
12-12: Add explicit access control to the plugin class.Suggested change
-final class DynamoDBPlugin: NSObject, TableProPlugin, DriverPlugin { +internal final class DynamoDBPlugin: NSObject, TableProPlugin, DriverPlugin {As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift` at line 12, The DynamoDBPlugin type declaration lacks an explicit access level; update the final class declaration for DynamoDBPlugin (which conforms to TableProPlugin and DriverPlugin) to include the appropriate access control (e.g., public or internal as required by consumers) so the class's visibility is explicit per the coding guideline.
127-128: Remove trailing comma.Suggested fix
section: .authentication - ), + ) ]As per coding guidelines: "no trailing commas".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift` around lines 127 - 128, Remove the trailing comma after the last element in the array literal that ends with ")," followed by "],"—locate the array construction around the closing brackets in DynamoDBPlugin (the sequence ")," then "],") and delete the extra comma so the final element is not followed by a comma, complying with the "no trailing commas" guideline.TableProTests/Plugins/DynamoDBStatementGeneratorTests.swift (1)
12-13: Add explicit access control to test struct.Suggested change
`@Suite`("DynamoDB Statement Generator") -struct DynamoDBStatementGeneratorTests { +internal struct DynamoDBStatementGeneratorTests {As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Plugins/DynamoDBStatementGeneratorTests.swift` around lines 12 - 13, The test type DynamoDBStatementGeneratorTests currently lacks an explicit access control modifier; update its declaration (the struct DynamoDBStatementGeneratorTests) to include the appropriate access level (e.g., mark it as internal or public based on test target conventions) so the type and any related extensions follow the guideline requiring explicit access control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swift`:
- Line 12: The DynamoDBPlugin type declaration lacks an explicit access level;
update the final class declaration for DynamoDBPlugin (which conforms to
TableProPlugin and DriverPlugin) to include the appropriate access control
(e.g., public or internal as required by consumers) so the class's visibility is
explicit per the coding guideline.
- Around line 127-128: Remove the trailing comma after the last element in the
array literal that ends with ")," followed by "],"—locate the array construction
around the closing brackets in DynamoDBPlugin (the sequence ")," then "],") and
delete the extra comma so the final element is not followed by a comma,
complying with the "no trailing commas" guideline.
In `@TableProTests/Plugins/DynamoDBQueryBuilderTests.swift`:
- Around line 11-31: The test struct DynamoDBQueryBuilderBrowseTests (and any
other test structs in this file) must declare explicit access control instead of
relying on the default; update the struct declaration for
DynamoDBQueryBuilderBrowseTests to include an access modifier (e.g., internal)
and do the same for any other test structs so types like
DynamoDBQueryBuilderBrowseTests and its test methods browseReturnsScanTag and
browseRoundTrip have explicit visibility per the coding guideline.
- Around line 50-52: Tests currently force-unwrap the optional `query` (e.g.,
`query!`) which can crash; update the assertions in the failing test(s) to
safely unwrap and assert presence before checking contents—use `try
`#require`(query)` or an `if let`/`guard let` pattern combined with `#expect` to
assert non-nil, then assert `query.hasPrefix(DynamoDBQueryBuilder.scanTag)` (and
similarly replace other force-unwrap sites referenced: lines 68, 88-93, 113-115,
172, 190, 210, 228, 277, 353) so tests fail gracefully with clear messages
rather than crashing.
In `@TableProTests/Plugins/DynamoDBStatementGeneratorTests.swift`:
- Around line 12-13: The test type DynamoDBStatementGeneratorTests currently
lacks an explicit access control modifier; update its declaration (the struct
DynamoDBStatementGeneratorTests) to include the appropriate access level (e.g.,
mark it as internal or public based on test target conventions) so the type and
any related extensions follow the guideline requiring explicit access control.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aea03c8e-a606-42a0-9930-3510daf924c6
📒 Files selected for processing (10)
Plugins/DynamoDBDriverPlugin/DynamoDBPlugin.swiftTablePro/AppDelegate.swiftTablePro/Core/Database/DatabaseDriver.swiftTablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swiftTablePro/Core/Storage/ConnectionStorage.swiftTablePro/Views/Connection/ConnectionFormView.swiftTableProTests/PluginTestSources/DynamoDBQueryBuilder.swiftTableProTests/PluginTestSources/DynamoDBStatementGenerator.swiftTableProTests/Plugins/DynamoDBQueryBuilderTests.swiftTableProTests/Plugins/DynamoDBStatementGeneratorTests.swift
✅ Files skipped from review due to trivial changes (2)
- TableProTests/PluginTestSources/DynamoDBQueryBuilder.swift
- TableProTests/PluginTestSources/DynamoDBStatementGenerator.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- TablePro/Core/Plugins/PluginMetadataRegistry+RegistryDefaults.swift
- TablePro/Views/Connection/ConnectionFormView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift (1)
464-466:⚠️ Potential issue | 🟡 MinorRaw response body may leak table data in error logs.
When the HTTP status is not 200 and JSON decoding fails, the raw response body is included in the thrown error. This could expose sensitive table contents in error messages shown to users or logged elsewhere.
🛡️ Suggested fix to redact response body
- let bodyStr = String(data: data, encoding: .utf8) ?? "No response body" - throw DynamoDBError.serverError("HTTP \(httpResponse.statusCode): \(bodyStr)") + throw DynamoDBError.serverError("HTTP \(httpResponse.statusCode): Response body redacted (length: \(data.count))")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift` around lines 464 - 466, The thrown error currently embeds the full HTTP response body (bodyStr) via DynamoDBError.serverError, which may leak sensitive table data; instead, sanitize the body before including it in the error by replacing the full body with a redacted summary (e.g., "<REDACTED>" or the first N characters + "…[REDACTED]") and/or include only metadata like content length and content-type; update the error construction that references bodyStr and httpResponse.statusCode so it uses the redacted string (and avoid returning raw body when String(data: data, encoding: .utf8) is nil) — locate the throw using DynamoDBError.serverError and variable bodyStr in DynamoDBConnection.swift and replace inclusion of the full body with a sanitized snippet.
🧹 Nitpick comments (3)
Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift (1)
84-90: Consider simplifying the double-optional unwrap pattern.The guard statement uses
val != nil, !val!.isEmptywhich, while safe due to the preceding check, is awkward. Using nested binding is cleaner.♻️ Suggested simplification
for key in keySchema { - guard let val = values[key.name], val != nil, !val!.isEmpty else { + guard let val = values[key.name], let unwrapped = val, !unwrapped.isEmpty else { Self.logger.warning("Skipping INSERT - missing key column '\(key.name)'") return [] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift` around lines 84 - 90, The guard in the for-loop over keySchema uses a double-optional check (val != nil, !val!.isEmpty); replace it with a single optional binding to simplify: bind the value with if/guard let (e.g., guard let val = values[key.name], !val.isEmpty) so you avoid force-unwrapping while preserving the existing behavior of logging via Self.logger.warning and returning [] from the loop; update the loop in DynamoDBStatementGenerator where keySchema and values are used to use this cleaner pattern.docs/databases/dynamodb.mdx (1)
319-326: Consider varying sentence structure to improve readability.Three consecutive bullet points begin with "No" (lines 319, 321, 324-326). While the content is accurate, varying the phrasing would improve flow.
✏️ Suggested rephrasing
-- **No schema editing.** DynamoDB does not support ALTER TABLE. Table structure is defined at creation time. +- **Schema editing unavailable.** DynamoDB does not support ALTER TABLE. Table structure is defined at creation time. - **Approximate item counts.** The item count shown in table metadata is approximate and updated by DynamoDB every ~6 hours. - **Cursor-based pagination.** DynamoDB uses continuation tokens internally. Jumping to an arbitrary page requires scanning through all preceding items. -- **No transactions UI.** DynamoDB transactions (TransactWriteItems/TransactGetItems) are not exposed in the UI. Use the AWS Console or CLI for transactional operations. -- **No DAX support.** DynamoDB Accelerator (DAX) in-memory caching is not supported. -- **No Global Tables management.** Multi-region Global Tables configuration is not available through the plugin. +- **Transactions not exposed.** DynamoDB transactions (TransactWriteItems/TransactGetItems) are not exposed in the UI. Use the AWS Console or CLI for transactional operations. +- **DAX unsupported.** DynamoDB Accelerator (DAX) in-memory caching is not supported. +- **Global Tables management unavailable.** Multi-region Global Tables configuration is not available through the plugin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/databases/dynamodb.mdx` around lines 319 - 326, The three consecutive bullets starting with "No" (the lines containing "No persistent connections.", "No SSH or SSL configuration.", and "No schema editing." as well as similar "No transactions UI.", "No DAX support.", "No Global Tables management.") should be reworded to vary sentence openings and improve flow; edit those list items so some lead with the positive subject or capability (e.g., "DynamoDB is HTTP-based — each operation is an independent API request." instead of "No persistent connections."), others with the mechanism ("Uses HTTPS transport with SigV4 signing; custom SSL certs or SSH tunnels are not required." instead of "No SSH or SSL configuration."), and some with the limitation framed differently ("Table structure is defined at creation time; ALTER TABLE is not supported." instead of "No schema editing."), keeping the factual content unchanged.Plugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swift (1)
94-117: Tokenizer may incorrectly parse PartiQL strings with embedded single quotes.PartiQL uses
''(doubled single quotes) to escape single quotes inside string literals, not\'. The current tokenizer only handles backslash escapes, which could cause incorrect token boundaries for strings like'O''Brien'.This may not be critical for table name extraction (the primary use case), but could cause issues with more complex PartiQL parsing.
♻️ Suggested enhancement for SQL-standard escaping
if char == "'" && !inDoubleQuote { - inSingleQuote.toggle() - current.append(char) - continue + current.append(char) + if inSingleQuote { + // Check for escaped quote ('') + // This requires lookahead, so handle in next iteration + inSingleQuote = false + } else { + inSingleQuote = true + } + continue }Note: A full fix would require lookahead to properly detect
''escapes. Consider if this is needed for the current use case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swift` around lines 94 - 117, The tokenizer currently treats backslash escapes (isEscaped) and toggles inSingleQuote on each single-quote char, which fails for PartiQL/SQL-style doubled single-quote escapes (e.g., 'O''Brien'); update the loop that iterates over sql (replace the char-for-in with an index-based loop so you can peek the next character) and modify the single-quote handling inside the loop: when char == '\'' and not inDoubleQuote, if inSingleQuote and nextChar == '\'' then append a single quote to current and advance the index (consuming the doubled quote) instead of toggling inSingleQuote; otherwise toggle inSingleQuote and append as before; keep the existing backslash logic for other escapes but do not treat \ as the SQL single-quote escape. Ensure you reference the existing variables isEscaped, inSingleQuote, inDoubleQuote, current while making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift`:
- Around line 36-37: The decoder currently maps invalid base64 to an empty
Data() in the initializer init(from decoder: Decoder) when decoding the enum
case .binary (using container.decodeIfPresent(String.self, forKey: .b)), causing
silent data loss; change this to decode the string, try Data(base64Encoded:),
and if that returns nil throw a DecodingError.dataCorruptedError (use
dataCorruptedError(forKey: .b, in: container, debugDescription: "Invalid base64
string")) so invalid base64 inputs fail decoding instead of producing empty
Data.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift`:
- Around line 1205-1232: The default branch in matchesFilter currently returns
true allowing unknown operators to match everything; change the default case to
return false so unknown/unsupported operators do not pass items, and add a
warning log (e.g., using your existing logger) in the default branch to record
the unrecognized op string for debugging; update the default logic inside the
matchesFilter(_ str: String, op: String, value: String) function accordingly.
---
Duplicate comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift`:
- Around line 464-466: The thrown error currently embeds the full HTTP response
body (bodyStr) via DynamoDBError.serverError, which may leak sensitive table
data; instead, sanitize the body before including it in the error by replacing
the full body with a redacted summary (e.g., "<REDACTED>" or the first N
characters + "…[REDACTED]") and/or include only metadata like content length and
content-type; update the error construction that references bodyStr and
httpResponse.statusCode so it uses the redacted string (and avoid returning raw
body when String(data: data, encoding: .utf8) is nil) — locate the throw using
DynamoDBError.serverError and variable bodyStr in DynamoDBConnection.swift and
replace inclusion of the full body with a sanitized snippet.
---
Nitpick comments:
In `@docs/databases/dynamodb.mdx`:
- Around line 319-326: The three consecutive bullets starting with "No" (the
lines containing "No persistent connections.", "No SSH or SSL configuration.",
and "No schema editing." as well as similar "No transactions UI.", "No DAX
support.", "No Global Tables management.") should be reworded to vary sentence
openings and improve flow; edit those list items so some lead with the positive
subject or capability (e.g., "DynamoDB is HTTP-based — each operation is an
independent API request." instead of "No persistent connections."), others with
the mechanism ("Uses HTTPS transport with SigV4 signing; custom SSL certs or SSH
tunnels are not required." instead of "No SSH or SSL configuration."), and some
with the limitation framed differently ("Table structure is defined at creation
time; ALTER TABLE is not supported." instead of "No schema editing."), keeping
the factual content unchanged.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swift`:
- Around line 94-117: The tokenizer currently treats backslash escapes
(isEscaped) and toggles inSingleQuote on each single-quote char, which fails for
PartiQL/SQL-style doubled single-quote escapes (e.g., 'O''Brien'); update the
loop that iterates over sql (replace the char-for-in with an index-based loop so
you can peek the next character) and modify the single-quote handling inside the
loop: when char == '\'' and not inDoubleQuote, if inSingleQuote and nextChar ==
'\'' then append a single quote to current and advance the index (consuming the
doubled quote) instead of toggling inSingleQuote; otherwise toggle inSingleQuote
and append as before; keep the existing backslash logic for other escapes but do
not treat \ as the SQL single-quote escape. Ensure you reference the existing
variables isEscaped, inSingleQuote, inDoubleQuote, current while making this
change.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swift`:
- Around line 84-90: The guard in the for-loop over keySchema uses a
double-optional check (val != nil, !val!.isEmpty); replace it with a single
optional binding to simplify: bind the value with if/guard let (e.g., guard let
val = values[key.name], !val.isEmpty) so you avoid force-unwrapping while
preserving the existing behavior of logging via Self.logger.warning and
returning [] from the loop; update the loop in DynamoDBStatementGenerator where
keySchema and values are used to use this cleaner pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8276fa08-70f5-4ab2-b26e-1e46d6bf881e
📒 Files selected for processing (9)
Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swiftPlugins/DynamoDBDriverPlugin/DynamoDBItemFlattener.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPartiQLParser.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swiftPlugins/DynamoDBDriverPlugin/DynamoDBStatementGenerator.swiftTableProTests/Plugins/DynamoDBStatementGeneratorTests.swiftdocs/databases/dynamodb.mdxdocs/vi/databases/dynamodb.mdxdocs/zh/databases/dynamodb.mdx
✅ Files skipped from review due to trivial changes (3)
- docs/vi/databases/dynamodb.mdx
- docs/zh/databases/dynamodb.mdx
- Plugins/DynamoDBDriverPlugin/DynamoDBItemFlattener.swift
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift (3)
28-28: Missing explicit access control on extension.As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🧹 Proposed fix
-extension DynamoDBAttributeValue: Codable { +internal extension DynamoDBAttributeValue: Codable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift` at line 28, The extension declaration "extension DynamoDBAttributeValue: Codable" lacks an explicit access level; update the extension to include the appropriate access modifier (e.g., public or internal) that matches the visibility of DynamoDBAttributeValue (for example "public extension DynamoDBAttributeValue: Codable" or "internal extension DynamoDBAttributeValue: Codable") so the extension's access is explicit and consistent with the type's visibility.
249-249: Missing explicit access control on class.As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🧹 Proposed fix
-final class DynamoDBConnection: `@unchecked` Sendable { +internal final class DynamoDBConnection: `@unchecked` Sendable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift` at line 249, The DynamoDBConnection class declaration lacks explicit access control; update the declaration (final class DynamoDBConnection: `@unchecked` Sendable) to include the intended access level (e.g., internal final class DynamoDBConnection: `@unchecked` Sendable or public final class DynamoDBConnection: `@unchecked` Sendable) and ensure any related extensions or nested types use the same or compatible access modifiers so the symbol visibility is consistent across the file.
108-228: Multiple types missing explicit access control.The following types should have explicit access control per coding guidelines:
AWSCredentials(line 108),DynamoDBError(line 116),ListTablesResponse(line 144),DescribeTableResponse(line 149),TableDescription(line 153),KeySchemaElement(line 168),AttributeDefinition(line 173),GlobalSecondaryIndexDescription(line 178),LocalSecondaryIndexDescription(line 188),ProvisionedThroughputDescription(line 196),BillingModeSummary(line 201),Projection(line 205),ScanResponse(line 210),QueryResponse(line 217),ExecuteStatementResponse(line 224).🧹 Example fix pattern
-struct AWSCredentials: Sendable { +internal struct AWSCredentials: Sendable {Apply the same pattern to all listed types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift` around lines 108 - 228, The listed types lack explicit access control; update each declaration to include an explicit access level (e.g., internal or public as appropriate for the module) — AWSCredentials, DynamoDBError, ListTablesResponse, DescribeTableResponse, TableDescription, KeySchemaElement, AttributeDefinition, GlobalSecondaryIndexDescription, LocalSecondaryIndexDescription, ProvisionedThroughputDescription, BillingModeSummary, Projection, ScanResponse, QueryResponse, and ExecuteStatementResponse — so change their declarations to start with the chosen modifier (preserve existing conformances like Error/LocalizedError/Decodable).Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift (1)
13-13: Missing explicit access control on class.As per coding guidelines: "Always specify access control explicitly (private, internal, public) on extensions and types."
🧹 Proposed fix
-final class DynamoDBPluginDriver: PluginDatabaseDriver, `@unchecked` Sendable { +internal final class DynamoDBPluginDriver: PluginDatabaseDriver, `@unchecked` Sendable {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift` at line 13, The class DynamoDBPluginDriver lacks an explicit access modifier; update its declaration to include the appropriate access control keyword (e.g., public, internal, or private) consistent with how DynamoDBPluginDriver and the PluginDatabaseDriver protocol are used in the module — for example change "final class DynamoDBPluginDriver: PluginDatabaseDriver, `@unchecked` Sendable" to include the chosen modifier (public/internal/private) so the type's visibility is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift`:
- Around line 53-54: The .binarySet decoding currently uses compactMap which
silently drops invalid base64 entries; update the decoding for key .bs in
DynamoDBConnection.swift to mirror the single .binary case by validating each
string: attempt Data(base64Encoded:) for every element, and if any conversion
fails throw a DecodingError.dataCorruptedError with a clear context instead of
filtering them out; ensure you assign self = .binarySet with the validated
[Data] only after all entries succeed so behavior is consistent with the binary
case.
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift`:
- Around line 666-716: executeScan applies client-side filters only after
fetching all pages which can yield fewer rows than fetchRows
(fetchMoreScanItems) that filters per-batch; make executeScan mirror
fetchMoreScanItems by applying applyClientFilters to each response batch before
appending so you continue fetching until you have fetchLimit filtered items or
no LastEvaluatedKey remains. Specifically, inside executeScan's repeat loop
(function executeScan) filter the local response.Items via applyClientFilters
using parsed.filters and parsed.logicMode, append only the filtered items to
allItems, and keep using fetchLimit/min(batchLimit) logic so
columns/typeNames/rows are derived from the same filtered dataset as used for
pagination; reference parsed.filters, parsed.limit/offset, fetchLimit,
lastEvaluatedKey, applyClientFilters, and fetchMoreScanItems for consistent
behavior.
- Around line 151-161: The current branch in DynamoDBPluginDriver uses only
parsed.filters.first when counting filtered scans; update the logic to support
all filters by adding a new helper (e.g., countFilteredScanItemsMultiple) that
accepts the full parsed.filters array and parsed.logic/mode, uses
applyClientFilters to apply every filter to the ScanExpression/Request, and
returns the correct count; replace the call to
countFilteredScanItems(tableName:parsed.tableName, conn:conn, filterColumn:...,
filterOp:..., filterValue:...) with a call to the new
countFilteredScanItemsMultiple(tableName: parsed.tableName, conn: conn, filters:
parsed.filters, logic: parsed.logic) and ensure countItems is still used when
parsed.filters is empty.
- Around line 128-130: The executeParameterized method currently ignores the
parameters array; update it to call DynamoDBConnection.executeStatement with a
parameters argument and propagate the parameters through the stack.
Specifically, change executeStatement (in DynamoDBConnection) to accept an
optional Parameters parameter (matching the AWS PartiQL "Parameters" field) and
include that field when building the ExecuteStatementInput/request to DynamoDB,
mapping each Swift String? parameter to the appropriate DynamoDB attribute value
(handling nils as NULL or expected AttributeValue types). Then modify
executeParameterized(query:parameters:) to forward the parameters to
executeStatement (instead of calling execute(query:)), and keep the return type
as PluginQueryResult so existing callers remain unchanged. Ensure function names
referenced: executeParameterized, execute(query:), executeStatement, and the
Parameters field in the ExecuteStatement request are updated accordingly.
---
Nitpick comments:
In `@Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swift`:
- Line 28: The extension declaration "extension DynamoDBAttributeValue: Codable"
lacks an explicit access level; update the extension to include the appropriate
access modifier (e.g., public or internal) that matches the visibility of
DynamoDBAttributeValue (for example "public extension DynamoDBAttributeValue:
Codable" or "internal extension DynamoDBAttributeValue: Codable") so the
extension's access is explicit and consistent with the type's visibility.
- Line 249: The DynamoDBConnection class declaration lacks explicit access
control; update the declaration (final class DynamoDBConnection: `@unchecked`
Sendable) to include the intended access level (e.g., internal final class
DynamoDBConnection: `@unchecked` Sendable or public final class
DynamoDBConnection: `@unchecked` Sendable) and ensure any related extensions or
nested types use the same or compatible access modifiers so the symbol
visibility is consistent across the file.
- Around line 108-228: The listed types lack explicit access control; update
each declaration to include an explicit access level (e.g., internal or public
as appropriate for the module) — AWSCredentials, DynamoDBError,
ListTablesResponse, DescribeTableResponse, TableDescription, KeySchemaElement,
AttributeDefinition, GlobalSecondaryIndexDescription,
LocalSecondaryIndexDescription, ProvisionedThroughputDescription,
BillingModeSummary, Projection, ScanResponse, QueryResponse, and
ExecuteStatementResponse — so change their declarations to start with the chosen
modifier (preserve existing conformances like Error/LocalizedError/Decodable).
In `@Plugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift`:
- Line 13: The class DynamoDBPluginDriver lacks an explicit access modifier;
update its declaration to include the appropriate access control keyword (e.g.,
public, internal, or private) consistent with how DynamoDBPluginDriver and the
PluginDatabaseDriver protocol are used in the module — for example change "final
class DynamoDBPluginDriver: PluginDatabaseDriver, `@unchecked` Sendable" to
include the chosen modifier (public/internal/private) so the type's visibility
is explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7ce1086-40e9-4668-9361-7f4118da958d
📒 Files selected for processing (2)
Plugins/DynamoDBDriverPlugin/DynamoDBConnection.swiftPlugins/DynamoDBDriverPlugin/DynamoDBPluginDriver.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New Files
Plugins/DynamoDBDriverPlugin/— 8 plugin source files + Info.plistTablePro/Assets.xcassets/dynamodb-icon.imageset/— AWS DynamoDB icondocs/databases/dynamodb.mdx(EN/VI/ZH)Modified Files
DatabaseConnection.swift— addedDatabaseType.dynamodbPluginMetadataRegistry+RegistryDefaults.swift— DynamoDB metadata snapshotConnectionFormView.swift— hide Database field and API Token for apiOnly plugins without database switching; fix validationMainContentCoordinator.swift— detect NoSQL query-building plugins for table name resolution (enables editing)SidebarViewModel.swift— debug logging (temporary)project.pbxproj— DynamoDBDriverPlugin target + bundled in app for testingdocs.json— navigation entries for all 3 languagesTest plan
docker run -p 8000:8000 amazon/dynamodb-localhttp://localhost:8000SELECT * FROM "Users"~/.aws/credentialsSummary by CodeRabbit
New Features
Documentation
UI