Skip to content

fix: Add validation to prevent duplicate adapter names #263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: Add validation to prevent duplicate adapter names #263

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84cbf7a
fix(fe): Add validation to prevent duplicate adapter names
claude[bot] Aug 27, 2025
79cc777
fix: Move adapter name validation from frontend to backend
claude[bot] Aug 27, 2025
a2afb57
fix: Ensure adapter name validation runs on every workflow call
claude[bot] Aug 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion services/budadmin/src/flows/AddAdapter/AdapterDetail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const AdapterDetail = () => {
})
}, [currentWorkflow])


const handleNext = async () => {
form.submit();
const result = await updateAdapterDetailWorkflow(adapterWorkflow?.adapterName, projectId);
Expand Down Expand Up @@ -64,7 +65,9 @@ export const AdapterDetail = () => {
name="adapterName"
label="Adapter deployment name"
placeholder="Enter adapter deployment name"
rules={[{ required: true, message: "Please enter deployment name" }]}
rules={[
{ required: true, message: "Please enter deployment name" }
]}
ClassNames="mt-[.4rem]"
infoText="Enter a name for the deployment of the adapter"
onChange={(e) => setAdapterWorkflow({ ...adapterWorkflow, "adapterName": e })}
Expand Down
3 changes: 2 additions & 1 deletion services/budapp/budapp/endpoint_ops/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,9 +1615,10 @@ async def add_adapter_workflow(self, current_user_id: UUID, request: AddAdapterR
{"name": adapter_name, "endpoint_id": endpoint_id},
missing_ok=True,
exclude_fields={"status": AdapterStatusEnum.DELETED},
case_sensitive=False,
)
if db_adapters:
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 👍 / 👎.

Copy link
Contributor Author

@dittops dittops Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@claude what's your take on this?

Copy link
Contributor

@claude claude bot Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Current Behavior: The code uses retrieve_by_fields(..., case_sensitive=False) which calls scalar_one_or_none() on the database query.

  2. SQLAlchemy's scalar_one_or_none() Behavior: This method will raise sqlalchemy.exc.MultipleResultsFound if the query returns more than one row.

  3. 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.

  4. Current Problem: Instead of getting a user-friendly validation error, users would get a 500 error with MultipleResultsFound exception.

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:

  1. The previous frontend-only validation was case-sensitive
  2. Users could have created adapters with names differing only in case
  3. The new backend validation would crash with MultipleResultsFound instead 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.

raise ClientException("Adapter name is already taken in the endpoint")
raise ClientException("An adapter with this name already exists")

db_endpoint = await EndpointDataManager(self.session).retrieve_by_fields(
EndpointModel,
Expand Down
Loading