DA-1171 list indexes returns list of index names and definitions #59
Conversation
Summary of ChangesHello @AayushTyagi1, 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 significantly enhances the toolset by introducing the ability to programmatically list and inspect Couchbase indexes. It provides a clear, structured output of index names, whether they are primary, and their complete N1QL definition, which is crucial for understanding and managing the database's indexing strategy. This addition improves observability and simplifies index management by allowing users to filter indexes based on their hierarchical location within the cluster. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to list Couchbase indexes, which is a valuable addition. The implementation is well-structured, with new modules for index-related functionality. I've identified a couple of areas for improvement: one is a minor code cleanup to remove a redundant variable, and the other is a more important fix to add validation in the index definition generation logic. This will make the function more robust against unexpected data and prevent potential errors or invalid SQL generation. Overall, great work on this feature.
| # Only generate definition for GSI indexes | ||
| if index_data.get("using") != "gsi": | ||
| return None | ||
|
|
||
| try: | ||
| # Start building the definition | ||
| if index_data.get("is_primary"): | ||
| query_definition = "CREATE PRIMARY INDEX" | ||
| else: | ||
| query_definition = "CREATE INDEX" | ||
|
|
||
| # Add index name | ||
| query_definition += f" `{index_data['name']}`" | ||
|
|
||
| # Add bucket name | ||
| query_definition += f" ON `{index_data['bucket']}`" | ||
|
|
||
| # Add scope and collection if they exist | ||
| scope = index_data.get("scope") | ||
| collection = index_data.get("collection") | ||
| if scope and collection: | ||
| query_definition += f".`{scope}`.`{collection}`" | ||
|
|
||
| # Add index keys for non-primary indexes | ||
| index_keys = index_data.get("index_key", []) | ||
| if index_keys and len(index_keys) > 0: | ||
| keys_str = ", ".join(str(key) for key in index_keys) | ||
| query_definition += f"({keys_str})" | ||
|
|
||
| # Add WHERE condition if exists | ||
| condition = index_data.get("condition") | ||
| if condition: | ||
| query_definition += f" WHERE {condition}" | ||
|
|
||
| # Add PARTITION BY if exists | ||
| partition = index_data.get("partition") | ||
| if partition: | ||
| query_definition += f" PARTITION BY {partition}" | ||
|
|
||
| return query_definition | ||
| except Exception as e: | ||
| logger.warning(f"Error generating index definition: {e}") | ||
| return None |
There was a problem hiding this comment.
The function could generate an invalid CREATE INDEX statement if name or bucket are missing or None in index_data. This can happen if the query to system:all_indexes returns incomplete data. It's safer to add explicit validation for these required fields at the beginning of the function to prevent generating invalid SQL and to make the function more robust. While the try...except block catches some errors, proactive validation is a better practice.
# Only generate definition for GSI indexes
if index_data.get("using") != "gsi":
return None
name = index_data.get("name")
bucket = index_data.get("bucket")
if not name or not bucket:
logger.warning(
f"Cannot generate index definition due to missing name or bucket. Data: {index_data}"
)
return None
try:
# Start building the definition
if index_data.get("is_primary"):
query_definition = "CREATE PRIMARY INDEX"
else:
query_definition = "CREATE INDEX"
# Add index name
query_definition += f" `{name}`"
# Add bucket name
query_definition += f" ON `{bucket}`"
# Add scope and collection if they exist
scope = index_data.get("scope")
collection = index_data.get("collection")
if scope and collection:
query_definition += f".`{scope}`.`{collection}`"
# Add index keys for non-primary indexes
index_keys = index_data.get("index_key", [])
if index_keys:
keys_str = ", ".join(str(key) for key in index_keys)
query_definition += f"({keys_str})"
# Add WHERE condition if exists
condition = index_data.get("condition")
if condition:
query_definition += f" WHERE {condition}"
# Add PARTITION BY if exists
partition = index_data.get("partition")
if partition:
query_definition += f" PARTITION BY {partition}"
return query_definition
except Exception as e:
logger.warning(f"Error generating index definition: {e}")
return None| temp_data = { | ||
| "name": index_data.get("name"), | ||
| "bucket": index_data.get("bucket_id"), | ||
| "scope": index_data.get("scope_id"), | ||
| "collection": index_data.get("keyspace_id"), | ||
| "index_type": index_data.get("using", "gsi"), | ||
| "is_primary": index_data.get("is_primary", False), | ||
| "index_key": index_data.get("index_key", []), | ||
| "condition": index_data.get("condition"), | ||
| "partition": index_data.get("partition"), | ||
| "using": index_data.get("using", "gsi"), | ||
| } |
There was a problem hiding this comment.
The temp_data dictionary includes a redundant index_type key. The using key is already present with the same value, and the generate_index_definition function only uses the using key. Removing the index_type key will improve code clarity.
temp_data = {
"name": index_data.get("name"),
"bucket": index_data.get("bucket_id"),
"scope": index_data.get("scope_id"),
"collection": index_data.get("keyspace_id"),
"is_primary": index_data.get("is_primary", False),
"index_key": index_data.get("index_key", []),
"condition": index_data.get("condition"),
"partition": index_data.get("partition"),
"using": index_data.get("using", "gsi"),
}
There was a problem hiding this comment.
Can we rename the using to type & then use it to generate the index definition?
| temp_data = { | ||
| "name": index_data.get("name"), | ||
| "bucket": index_data.get("bucket_id"), | ||
| "scope": index_data.get("scope_id"), | ||
| "collection": index_data.get("keyspace_id"), | ||
| "index_type": index_data.get("using", "gsi"), | ||
| "is_primary": index_data.get("is_primary", False), | ||
| "index_key": index_data.get("index_key", []), | ||
| "condition": index_data.get("condition"), | ||
| "partition": index_data.get("partition"), | ||
| "using": index_data.get("using", "gsi"), | ||
| } |
There was a problem hiding this comment.
Can we rename the using to type & then use it to generate the index definition?
| logger = logging.getLogger(f"{MCP_SERVER_NAME}.utils.index_utils") | ||
|
|
||
|
|
||
| def generate_index_definition(index_data: dict[str, Any]) -> str | None: |
There was a problem hiding this comment.
Is there an easier way to get this data? The concern here is that there might be more parameters than the ones we parse like in the case of vector search such as with.
Did you try the management API in the SDK to see if it has something out of the box?
There was a problem hiding this comment.
I don't think anything return Index Definition in SDK though management API. I can recheck if some update is there. We are using the same approach in VS Code and jetbrains for this reason only. I will recheck.
There was a problem hiding this comment.
I guess you will have a merge conflict with #58.
There was a problem hiding this comment.
Yes I am aware of that. I will merge main once other one get merged.
No description provided.