Skip to content

feat: add enhanced RAG validation with comprehensive checks and utili… - #13

Open
jaffrey-deepsource wants to merge 1 commit into
mainfrom
feature/enhanced-rag-validation
Open

feat: add enhanced RAG validation with comprehensive checks and utili…#13
jaffrey-deepsource wants to merge 1 commit into
mainfrom
feature/enhanced-rag-validation

Conversation

@jaffrey-deepsource

Copy link
Copy Markdown
Collaborator

…ties

  • Add validation for dataset retrieval config in backend
  • Add relevance score calculation and query validation helpers
  • Add document filtering and averaging utilities
  • Add frontend validation utilities and hooks for RAG pipeline
  • Add pipeline validator class for enhanced checks
  • Add rerank parameter validation and scoring
  • Add embedding input validation and similarity computation
  • Add text splitter parameter validation and chunk estimation
  • Add dataset config validation and statistics computation
  • Add document validation and content utilities
  • Add index parameter validation and size estimation
  • Add RAG pipeline config validation and metrics calculation

…ties

- Add validation for dataset retrieval config in backend
- Add relevance score calculation and query validation helpers
- Add document filtering and averaging utilities
- Add frontend validation utilities and hooks for RAG pipeline
- Add pipeline validator class for enhanced checks
- Add rerank parameter validation and scoring
- Add embedding input validation and similarity computation
- Add text splitter parameter validation and chunk estimation
- Add dataset config validation and statistics computation
- Add document validation and content utilities
- Add index parameter validation and size estimation
- Add RAG pipeline config validation and metrics calculation
@github-actions github-actions Bot added the web label Feb 11, 2026
@deepsource-development

deepsource-development Bot commented Feb 11, 2026

Copy link
Copy Markdown

DeepSource Code Review

DeepSource reviewed changes in the commit range b76c8fa..63debb2 on this pull request. Below is the summary for the review, and you can see the individual issues we found as review comments.

For detailed review results, please see the PR on DeepSource ↗

PR Report Card

Security × 7 issues Overall PR Quality   

Focus Area: Reliability

Guidance
Fix 7 critical unreturned function call assignments in `api/services/dataset_service.py`.

Grade capped at D due to multiple critical issues
Reliability × 26 issues
Complexity × 40 issues
Hygiene × 22 issues

Code Review Summary

Analyzer Status Summary Details
JavaScript 14 new issues detected. Review ↗
Python 82 new issues detected. Review ↗
Secrets No new issues detected. Review ↗
How are these analyzer statuses calculated?

Administrators can configure which issue categories are reported and cause analysis to be marked as failed when detected. This helps prevent bad and insecure code from being introduced in the codebase. If you're an administrator, you can modify this in the repository's settings.


💡 If you're a repository administrator, you can configure the quality gates from the settings.

if top_n is not None and top_n <= 0:
raise ValueError("Top N must be positive")

def compute_rerank_score(self, query: str, document: Document) -> float:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instance method without `self` usage wastes memory


The method compute_rerank_score is defined as an instance method but does not use the self parameter. This causes Python to bind the method to each instance, which wastes memory and increases call overhead.
Add the @staticmethod decorator to compute_rerank_score to avoid binding to instances, improving memory use and performance.

if not text or len(text.strip()) == 0:
raise ValueError("All texts must be non-empty")

def compute_embedding_similarity(self, embedding1: list[float], embedding2: list[float]) -> float:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instance method unused; decorate with `@staticmethod` to save resources


The method compute_embedding_similarity is defined with self but does not use any instance variables or state. This causes Python to create a bound method every time, consuming unnecessary resources.

Add the @staticmethod decorator to this method so it is treated as a static method. This avoids binding self and improves performance by reducing overhead.

def format_preview(self, chunks: Any) -> Mapping[str, Any]:
raise NotImplementedError

def validate_index_params(self, dataset: Dataset, top_k: int, score_threshold: float) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Method lacks `@staticmethod`, adding unnecessary bound instance


The method validate_index_params is an instance method but does not use the self parameter, causing Python to create a bound method unnecessarily for each class instance. This wastes memory and computation time.

Add the @staticmethod decorator above validate_index_params to avoid binding it to instances and improve performance.

:return: Value or None
"""
return self.metadata.get(key) if self.metadata else None
"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

String statement has no effect


The string statement has not been assigned to anything. This is pointless and should be removed if not necessary. In case this is supposed to describe what's happening in the code, it is recommended to use comments or docstrings instead.

raise ValueError("Top K cannot exceed 100 to prevent excessive resource usage")

# Validate score threshold if enabled
if retrieve_config.score_threshold_enabled:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nested `if` statements can be merged for clarity


Nested if statements where conditions can be combined add unnecessary indentation and reduce code clarity. Collapsing them using a single if with an and logical operator improves maintainability and readability.

Merge the nested if statements by combining their conditions with the and operator to simplify the control structure and reduce nesting.

if not rerank_model.reranking_provider_name or not rerank_model.reranking_model_name:
raise ValueError("Reranking model must be fully specified when reranking is enabled")

def _calculate_relevance_score(self, documents: list[Document], query: str) -> float:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instance method not using `self` wastes memory and computation


The method _calculate_relevance_score is defined with self but does not reference it, causing Python to create a bound method for each class instance unnecessarily. This wastes memory and CPU resources when many instances are created.

Add the @staticmethod decorator above this method to remove the implicit self parameter and avoid binding it to instances.


return total_score / len(documents)

def _validate_query_input(self, query: str, inputs: Mapping[str, Any] | None) -> None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instance method without use of `self` wastes memory


The _validate_query_input method is defined with a self parameter but does not use it, resulting in Python creating a bound method for every class instance, which wastes memory and minor CPU cycles. This inefficiency occurs because the method does not require instance context.

Add the @staticmethod decorator above the method definition to remove the need for a bound instance, improving memory and runtime efficiency.

db.session.commit()
except Exception as e:
db.session.rollback()
raise e

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Undefined variable 'e'


The variable name is not defined where it is used.
This will lead to an error during the runtime.
Make sure there is no typo. If the name was supposed to be imported, verify that you've actually imported the name.

Comment on lines +104 to +105
if top_k <= 0:
raise ValueError("Top K must be positive")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`top_k` parameter lacks an upper-bound check


The validate_index_params method checks that top_k is positive but fails to enforce an upper limit. This could allow a very large top_k value to be passed, potentially causing denial-of-service by consuming excessive memory or CPU during data retrieval.

Add an upper-bound check to top_k, for example if not (0 &lt; top_k &lt;= 100):, to align with limits in other parts of the system and prevent resource exhaustion.

total_score = 0.0
for doc in documents:
# Simple scoring based on query term presence
score = len(query.split()) / len(doc.page_content.split()) if doc.page_content else 0.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`ZeroDivisionError` if `doc.page_content` contains only whitespace


The scoring calculation at line 143 divides by the length of words from doc.page_content. If doc.page_content contains only whitespace (e.g., ' '), it is considered truthy, but doc.page_content.split() results in an empty list. This leads to a ZeroDivisionError, which can crash the service, causing a denial of service.

To fix this, first split the content and then check if the resulting list is non-empty before performing the division. This ensures the application remains stable even with malformed document content.

}
}

const handleChange = (key: keyof RagValidationConfig, value: any) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`any` type disables TypeScript type checking


The parameter value is typed as any in the handleChange function, which disables TypeScript's static type checking and allows any value type without restrictions. This bypass can lead to runtime errors and harder-to-maintain code.

Replace any with a more specific type or use unknown to enforce type checks before usage, maintaining type safety and preventing unexpected values.

type="checkbox"
id="strict-validation"
checked={localConfig.enableStrictValidation}
onChange={(e) => handleChange('enableStrictValidation', e.target.checked)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inline callback in JSX causes unnecessary re-renders


Defining (e) =&gt; handleChange(&#x27;enableStrictValidation&#x27;, e.target.checked) inline causes a new function instance every render, triggering unnecessary React re-renders in child components using props shallow comparison.
Use React.useCallback to memoize the callback or define the handler outside the render function to maintain stable references.

type="number"
id="max-topk"
value={localConfig.maxTopK}
onChange={(e) => handleChange('maxTopK', parseInt(e.target.value))}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inline arrow function as prop causes re-creation on each render


The inline arrow function (e) =&gt; handleChange(&#x27;maxTopK&#x27;, parseInt(e.target.value)) is recreated each time the component renders, causing potential inefficient re-renders in React components that optimize with reference equality checks. This overhead can accumulate in frequently rendered components.

Use React.useCallback to memoize the callback or define the handler function outside the component render to maintain stable references and avoid unnecessary re-renders.

type="checkbox"
id="require-score-threshold"
checked={localConfig.requireScoreThreshold}
onChange={(e) => handleChange('requireScoreThreshold', e.target.checked)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inline callback recreates on every render reducing performance


Creating an inline callback in JSX properties, like (e) =&gt; handleChange(&#x27;requireScoreThreshold&#x27;, e.target.checked), causes a new function reference on each render. Components receiving this prop may re-render unnecessarily if they rely on shallow prop comparison for optimization.

Use React.useCallback to memoize the handler or define the callback outside the render scope. This prevents costly re-creations and helps maintain performant renders.

warnings: string[]
}

export const validateRagPipelineConfig = (config: any): RagValidationResult => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using `any` disables TypeScript type checking


Typing config as any disables compile-time checks for the parameter, allowing runtime errors and harder-to-maintain code. This undermines TypeScript's type safety benefits.

Replace any with a specific type, unknown, or a well-defined interface to ensure proper type checking and safer code.

return totalScore / documents.length
}

export const filterDocuments = (documents: any[], threshold: number): any[] => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Use of `any` type disables type safety checks


The parameter documents and the return type are typed as any[], which disables TypeScript's static type checking and allows any data shape to pass unchecked. This can cause bugs or unexpected errors when filterDocuments operates on assumed types.
Replace any types with more specific or safer alternatives like unknown[] or properly typed arrays. This maintains type safety and improves code reliability.

}

const RagPipelineValidation = ({ config, onConfigChange }: RagPipelineValidationProps) => {
const [localConfig, setLocalConfig] = useState(config)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Component state initialized from props does not update when props change


The component's internal state localConfig is initialized from the config prop only once, during the initial render. If the parent component later passes a new config prop, this component will not update to reflect the new values, leading to a desynchronized and stale UI.

Use the useEffect hook to synchronize the internal state with the config prop whenever it changes. This ensures the component always reflects the most current configuration provided by its parent.

type="number"
id="max-topk"
value={localConfig.maxTopK}
onChange={(e) => handleChange('maxTopK', parseInt(e.target.value))}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`parseInt` on an empty or invalid string returns `NaN`, which bypasses validation


The onChange handler for the maxTopK input uses parseInt on the input value. If a user clears the input or enters non-numeric text, e.target.value becomes &#x27;&#x27; or an invalid string, and parseInt returns NaN. This NaN value bypasses the validation logic and corrupts the component's state, which is then propagated to the parent.

Modify the onChange handler to check if the parsed value is NaN. If it is, the invalid input should be ignored to prevent state corruption. Additionally, it is best practice to provide the radix parameter to parseInt.

Comment on lines +8 to +16
export const useRagValidation = (config: any) => {
const [validation, setValidation] = useState<RagValidationState>({ isValid: true, errors: [] })

useEffect(() => {
const errors: string[] = []

if (config.topK <= 0) errors.push('Top K must be positive')
if (config.topK > 100) errors.push('Top K cannot exceed 100')
if (config.scoreThreshold < 0 || config.scoreThreshold > 1) errors.push('Score threshold must be between 0 and 1')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

`config` uses `any` type and its properties are accessed without validation


The config parameter is typed as any, which disables TypeScript's static analysis and type safety. Additionally, its properties are accessed without runtime validation, which can cause a TypeError if config is null or undefined, or lead to incorrect validation if properties like topK are missing or not numbers.

Define a specific interface for config and add runtime checks to ensure config and its properties are valid before use. This will prevent crashes and ensure the validation logic is reliable.

Comment on lines +44 to +53
const totalDocs = documents.length
const avgLength = documents.reduce((sum, doc) => sum + (doc.content?.length || 0), 0) / totalDocs
const relevantDocs = documents.filter(doc => doc.score > 0.5).length

return {
totalDocuments: totalDocs,
averageContentLength: avgLength,
relevantDocuments: relevantDocs,
relevanceRatio: relevantDocs / totalDocs
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Division by `totalDocs` can result in `NaN` if `documents` is empty


The calculations for avgLength and relevanceRatio do not handle the case where the documents array is empty. This results in a division by zero, producing NaN values that can cause silent failures or display issues elsewhere in the application.

Add a guard clause or use a ternary operator to check if totalDocs is zero. If it is, return 0 for averageContentLength and relevanceRatio to prevent NaN propagation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant