exit app target column #665
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the database schema, specifically the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/discovery-platform/src/node.spec.ts (1)
Line range hint 82-117: Consider adding specific tests for exit_app_target functionality
The current test suite focuses on pairing functionality but doesn't explicitly verify how the new exit_app_target field affects the node selection or routing behavior. Consider adding test cases that:
- Verify the application correctly uses the
exit_app_targetvalue - Test edge cases in target resolution
- Ensure backward compatibility with existing nodes
Would you like me to help draft additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apps/discovery-platform/db.schema (2 hunks)
- apps/discovery-platform/migrations/1730194865328_add-exit-app-target.js (1 hunks)
- apps/discovery-platform/src/node.spec.ts (1 hunks)
- apps/discovery-platform/src/node.ts (4 hunks)
🔇 Additional comments (10)
apps/discovery-platform/migrations/1730194865328_add-exit-app-target.js (4)
10-13: LGTM! Down migration correctly reverts changes.
The down migration properly reverses the changes in the correct order:
- Renames the column back
- Drops the new column
This ensures a clean rollback if needed.
1-13: Consider adding indexes for performance.
Since this appears to be a key field used for lookups (based on the naming), consider adding an index on the exit_app_pub_key column if it doesn't already exist.
#!/bin/bash
# Check for existing indexes on the table
psql \
-c "SELECT
t.relname AS table_name,
i.relname AS index_name,
a.attname AS column_name
FROM
pg_class t,
pg_class i,
pg_index ix,
pg_attribute a
WHERE
t.oid = ix.indrelid
AND i.oid = ix.indexrelid
AND a.attrelid = t.oid
AND a.attnum = ANY(ix.indkey)
AND t.relkind = 'r'
AND t.relname = 'registered_nodes';"7-7: Verify no foreign key constraints on the renamed column.
The column rename operation looks correct. However, we should verify there are no foreign key constraints referencing this column that might need to be updated.
#!/bin/bash
# Check for foreign key constraints referencing the exit_node_pub_key column
psql \
-c "SELECT
tc.table_schema,
tc.constraint_name,
tc.table_name,
kcu.column_name
FROM
information_schema.table_constraints AS tc
JOIN information_schema.key_column_usage AS kcu
ON tc.constraint_name = kcu.constraint_name
AND tc.table_schema = kcu.table_schema
WHERE tc.constraint_type = 'FOREIGN KEY'
AND kcu.column_name = 'exit_node_pub_key';"4-7: Consider adding NOT NULL constraint with a default value.
The new exit_app_target column is added without any constraints. For existing rows, this will be NULL. Consider whether this column should have a NOT NULL constraint with a default value to maintain data integrity.
apps/discovery-platform/db.schema (1)
202-202: LGTM: Added newline at end of file.
This is a good practice to ensure proper file formatting.
apps/discovery-platform/src/node.spec.ts (1)
25-29: Verify the format of exit_app_target test data
The test data uses a consistent pattern of pubtargetN:1234 for the new exit_app_target column. While this covers the basic case, consider adding test cases with:
- Different port numbers to verify port handling
- Invalid formats to ensure proper validation
- Edge cases like missing ports or malformed targets
Let's verify the expected format by checking the schema and related code:
apps/discovery-platform/src/node.ts (4)
19-19: LGTM: ExitNode type extension
The addition of the target field to the ExitNode type properly reflects the schema changes.
36-37: LGTM: DBExitNode type updates
The type changes correctly mirror the database schema modifications, maintaining proper naming conventions.
67-67: LGTM: Updated query columns
The SQL query has been properly updated to select the renamed and new columns while maintaining security practices.
123-124: LGTM: Updated field mapping
The mapping function correctly transforms the database fields to their corresponding API type properties.
Summary by CodeRabbit
New Features
exit_app_targetin the registered nodes, enhancing data tracking capabilities.exit_node_pub_keytoexit_app_pub_keyfor improved clarity.Bug Fixes
Documentation