fix: Add validation to prevent duplicate adapter names #263
Conversation
- Add useEndPoints hook to fetch existing adapters - Implement validateAdapterName function for case-insensitive duplicate checking - Update TextInput rules to include duplicate validation - Prevents users from creating adapters with same names within an endpoint Fixes #251 Co-authored-by: Ditto P S <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @dittops, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces frontend validation to prevent users from creating new adapters with names that already exist within the same endpoint. This enhancement ensures data integrity and improves user experience by providing immediate feedback on duplicate entries.
Highlights
- Duplicate Name Validation: Implemented case-insensitive validation for adapter names within the AdapterDetail component.
- Existing Adapter Fetching: The system now fetches existing adapters to perform accurate validation against current entries.
- User Feedback: A clear error message is displayed to the user when a duplicate adapter name is detected.
- Form Submission Prevention: The form is prevented from being submitted if an invalid (duplicate) adapter name is entered.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
@claude The validation was done on the frontend, which is not correct. You should do this on the backend/budapp properly. |
There was a problem hiding this comment.
Code Review
This pull request adds frontend validation to prevent creating adapters with duplicate names, which is a great improvement. The implementation correctly uses a custom validator with antd Form.
My review includes two main points:
- A potential issue where not all existing adapters are fetched for validation due to a hardcoded limit, which could allow duplicates if there are more than 100 adapters.
- A suggestion to optimize the validation function using
useCallbackand add type annotations for better performance and maintainability.
Addressing these points will make the validation more robust and the code more efficient.
| useEffect(() => { | ||
| if (currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id) { | ||
| const endpointId = currentWorkflow.workflow_steps.adapter_config.endpoint_id; | ||
| getAdapters({ | ||
| endpointId: endpointId, | ||
| page: 1, | ||
| limit: 100, // Get all adapters for validation | ||
| }, projectId); | ||
| } | ||
| }, [currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id, projectId]) |
There was a problem hiding this comment.
The useEffect hook for fetching adapters has a hardcoded limit of 100. This could lead to incorrect duplicate name validation if there are more than 100 adapters for an endpoint, as not all existing adapters would be fetched. A more robust solution would be to implement pagination to retrieve all adapters.
Additionally, the getAdapters function should be included in the useEffect dependency array to prevent potential stale closures and adhere to the exhaustive-deps linting rule.
| useEffect(() => { | |
| if (currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id) { | |
| const endpointId = currentWorkflow.workflow_steps.adapter_config.endpoint_id; | |
| getAdapters({ | |
| endpointId: endpointId, | |
| page: 1, | |
| limit: 100, // Get all adapters for validation | |
| }, projectId); | |
| } | |
| }, [currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id, projectId]) | |
| useEffect(() => { | |
| if (currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id) { | |
| const endpointId = currentWorkflow.workflow_steps.adapter_config.endpoint_id; | |
| getAdapters({ | |
| endpointId: endpointId, | |
| page: 1, | |
| limit: 1000, // TODO: Fetch all adapters for complete validation. This is a temporary higher limit. | |
| }, projectId); | |
| } | |
| }, [currentWorkflow?.workflow_steps?.adapter_config?.endpoint_id, projectId, getAdapters]) |
| const validateAdapterName = (_, value) => { | ||
| if (!value) { | ||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| // Check if the name already exists (case-insensitive) | ||
| const existingAdapter = adapters.find(adapter => | ||
| adapter.name.toLowerCase() === value.toLowerCase() | ||
| ); | ||
|
|
||
| if (existingAdapter) { | ||
| return Promise.reject(new Error('An adapter with this name already exists')); | ||
| } | ||
|
|
||
| return Promise.resolve(); | ||
| }; |
There was a problem hiding this comment.
The validateAdapterName function is redefined on every render. For better performance, it should be memoized using useCallback. Also, adding type annotations for the function parameters would improve type safety and code clarity.
Note: You'll need to import useCallback from react.
| const validateAdapterName = (_, value) => { | |
| if (!value) { | |
| return Promise.resolve(); | |
| } | |
| // Check if the name already exists (case-insensitive) | |
| const existingAdapter = adapters.find(adapter => | |
| adapter.name.toLowerCase() === value.toLowerCase() | |
| ); | |
| if (existingAdapter) { | |
| return Promise.reject(new Error('An adapter with this name already exists')); | |
| } | |
| return Promise.resolve(); | |
| }; | |
| // Validation function to check for duplicate names | |
| const validateAdapterName = useCallback((_: any, value: string) => { | |
| if (!value) { | |
| return Promise.resolve(); | |
| } | |
| // Check if the name already exists (case-insensitive) | |
| const existingAdapter = adapters.find(adapter => | |
| adapter.name.toLowerCase() === value.toLowerCase() | |
| ); | |
| if (existingAdapter) { | |
| return Promise.reject(new Error('An adapter with this name already exists')); | |
| } | |
| return Promise.resolve(); | |
| }, [adapters]); |
|
Claude finished @dittops's task —— View job I've successfully moved the adapter name validation from frontend to backend as requested. Todo List
Changes Made
SummaryThe validation is now properly handled on the backend where it belongs. The backend performs case-insensitive duplicate name checking and returns a 400 error with the message "An adapter with this name already exists" when a duplicate is detected. The frontend will display this error message through its existing error handling mechanism, providing a better separation of concerns. The changes have been committed and pushed to the branch: |
- Updated backend validation in budapp to use case-insensitive comparison - Added case_sensitive=False parameter to AdapterDataManager.retrieve_by_fields call - Updated error message to be consistent with frontend expectations - Removed frontend validation logic from AdapterDetail.tsx component - Removed unused imports and adapter fetching code from frontend Co-authored-by: Ditto P S <[email protected]>
|
@codex review this pr |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| {"name": adapter_name, "endpoint_id": endpoint_id}, | ||
| missing_ok=True, | ||
| exclude_fields={"status": AdapterStatusEnum.DELETED}, | ||
| case_sensitive=False, | ||
| ) | ||
| if db_adapters: |
There was a problem hiding this comment.
[P1] Handle multiple case-insensitive matches gracefully
The new call to retrieve_by_fields(..., case_sensitive=False) now collapses name comparison to lowercase, but retrieve_by_fields still uses scalar_one_or_none. If the database already contains two adapters whose names differ only by casing (e.g. "Adapter" and "adapter", which was previously allowed because validation was case-sensitive), this query will now raise MultipleResultsFound and bubble up as a 500 instead of returning a ClientException. The change prevents new duplicates, but it makes the API fail for existing data rather than surfacing a user-facing validation error. Consider retrieving all matches or constraining the query to a single row before raising the custom exception.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude finished @dittops's task —— View job
Analyzing the Codex review concern about case-insensitive validation...
Todo List
- Read the current implementation in services/budapp/budapp/endpoint_ops/services.py
- Understand how retrieve_by_fields works with case_sensitive=False
- Assess the validity of the MultipleResultsFound concern
- Provide recommendations for handling the edge case
Analysis and Assessment
Codex is absolutely correct about this concern. Here's the detailed analysis:
The Problem
-
Current Behavior: The code uses
retrieve_by_fields(..., case_sensitive=False)which callsscalar_one_or_none()on the database query. -
SQLAlchemy's scalar_one_or_none() Behavior: This method will raise
sqlalchemy.exc.MultipleResultsFoundif the query returns more than one row. -
The Edge Case: If the database already contains adapters with names that differ only by case (e.g., "Adapter" and "adapter"), the case-insensitive query
func.lower(field) == func.lower(value)would match both records. -
Current Problem: Instead of getting a user-friendly validation error, users would get a 500 error with
MultipleResultsFoundexception.
Database Schema Analysis
Looking at the adapter table creation in migration 35c352d33c91_adapter_table_workflow_type.py, I can see:
- No unique constraint on the
(name, endpoint_id)combination - No database-level case-insensitive constraint
- The previous case-sensitive validation allowed "Adapter" and "adapter" to coexist in the same endpoint
The Risk
This is a real production risk because:
- The previous frontend-only validation was case-sensitive
- Users could have created adapters with names differing only in case
- The new backend validation would crash with
MultipleResultsFoundinstead of providing helpful validation
Recommendation
Option 1: Robust Query Approach (Recommended)
Replace the current logic with:
# Use scalars_all() to get all matching adapters
db_adapters = await AdapterDataManager(self.session).scalars_all(
select(AdapterModel).filter(
func.lower(AdapterModel.name) == func.lower(adapter_name),
AdapterModel.endpoint_id == endpoint_id,
AdapterModel.status != AdapterStatusEnum.DELETED
)
)
if db_adapters:
raise ClientException("An adapter with this name already exists")Option 2: Try-Catch Approach
Wrap the current code in try-catch to handle MultipleResultsFound:
try:
db_adapters = await AdapterDataManager(self.session).retrieve_by_fields(...)
if db_adapters:
raise ClientException("An adapter with this name already exists")
except MultipleResultsFound:
raise ClientException("An adapter with this name already exists")I recommend Option 1 as it's cleaner and more explicit about the intention.
|
@claude the issue reported is not casesensitve names. even though the name used previously, when tried to use the same name again, it accepted and started the lora deployment process.. Think harder and fix this |
|
Claude finished @dittops's task —— View job I've successfully identified and fixed the adapter name validation bug! Todo List
Root Cause Found! 🎯I've identified two critical issues in the current validation logic: Issue #1: Validation Bypass Bug (Main Issue)The validation logic at lines
This explained why "even though the name used previously, when tried to use the same name again, it accepted and started the lora deployment process." Issue #2: MultipleResultsFound Exception RiskThe previous validation used Solution Implemented ✅I've fixed both issues by:
Changes Made
The validation now properly prevents duplicate adapter names on every attempt, not just the first one! SummaryThe issue was a logic bug where adapter name validation only ran once during workflow creation, but was bypassed on subsequent retries. The fix ensures validation runs consistently on every adapter creation attempt. |
- Fix validation bypass bug where duplicate names were accepted on retry attempts - Move validation from one-time execution to every workflow call - Add proper _validate_adapter_name helper method using scalars_all() - Prevent MultipleResultsFound exceptions from case-insensitive queries - Ensure validation runs in both existing and new workflow step branches Co-authored-by: Ditto P S <[email protected]>
Fixes #251
Adds frontend validation to prevent users from creating adapters with duplicate names within the same endpoint.
Changes
Generated with Claude Code