-
Notifications
You must be signed in to change notification settings - Fork 0
Implement filter multiple columns API #45
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { | ||
| filterMultipleColumns, | ||
| validateFilter | ||
| } from "@/lib/api/filterMultipleColumns"; | ||
|
|
||
| export async function GET(request: NextRequest) { | ||
| try { | ||
| const searchParams = request.nextUrl.searchParams; | ||
|
|
||
| const filtersListParameter = searchParams.get("filters_list"); | ||
| const op = searchParams.get("op"); | ||
|
|
||
| if (!filtersListParameter || !op) | ||
| return NextResponse.json({ error: "Missing parameter" }, { status: 400 }); | ||
|
|
||
| let filters; | ||
| try { | ||
| filters = JSON.parse(filtersListParameter); | ||
| } catch { | ||
| return NextResponse.json( | ||
| { error: "Invalid JSON for 'filters_list' parameter" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| const validation = validateFilter(filters, op); | ||
| if (!validation.valid) | ||
| return NextResponse.json({ error: validation.error }, { status: 400 }); | ||
|
|
||
| const { data, error } = await filterMultipleColumns(filters, op); | ||
|
|
||
| if (error) return NextResponse.json({ error }, { status: 500 }); | ||
|
|
||
| return NextResponse.json({ data }, { status: 200 }); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : "Unexpected error"; | ||
|
|
||
| return NextResponse.json({ error: message }, { status: 500 }); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,221 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createClient } from "../client/supabase/server"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { Database } from "../client/supabase/types"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export type FilterTuple = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mini_op: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| field: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| values: string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| values: string[]; | |
| values: string[] | [string, string][]; |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message inconsistency: the validation function returns "'filter' must be an array" (line 39) but the parameter is actually named 'filters_list' in the API route. This could confuse API consumers. Consider using the actual parameter name in the error message.
| return { valid: false, error: "'filter' must be an array" }; | |
| return { valid: false, error: "'filtersList' must be an array" }; |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Unnecessary empty comment or unclear intention. If this comment is meant to explain why empty arrays are allowed, it should be more explicit (e.g., "Empty filter arrays are allowed and will return all volunteers"). Otherwise, remove the empty comment.
| // Allow filter to be an empty array | |
| // Empty filter arrays are allowed and will return all volunteers |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Case sensitivity inconsistency: The validation converts field names to lowercase (line 47) and the filtering logic also uses lowercase (line 96), but mini_op is validated case-sensitively (line 44). Consider normalizing mini_op to uppercase before validation for consistency and better user experience.
| if (!f.mini_op || !(f.mini_op == "AND" || f.mini_op == "OR")) | |
| if ( | |
| !f.mini_op || | |
| !(f.mini_op.toUpperCase() === "AND" || f.mini_op.toUpperCase() === "OR") | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking f.mini_op == "AND" every time, you can create operation constants to improve consistency and reduce typos:
export const MINI_OP = {
AND: "AND",
OR: "OR",
} as const;
Then do f.mini_op == MINI_OP.AND. Same goes to OR.
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The cohort filter values use a string year that must be parseable as an integer, but the database query expects an integer. While parseInt(v[1]) is used in the query (line 160), the validation should ensure the parsed value is within a reasonable range (e.g., 1900-2100) to prevent potential issues with edge cases or malformed data.
| (v) => | |
| !Array.isArray(v) || | |
| v.length !== 2 || | |
| typeof v[0] !== "string" || | |
| !/^(Fall|Spring|Summer|Winter)$/i.test(v[0]) || | |
| typeof v[1] !== "string" || | |
| isNaN(parseInt(v[1])) | |
| (v) => { | |
| if ( | |
| !Array.isArray(v) || | |
| v.length !== 2 || | |
| typeof v[0] !== "string" || | |
| !/^(Fall|Spring|Summer|Winter)$/i.test(v[0]) || | |
| typeof v[1] !== "string" | |
| ) { | |
| return true; | |
| } | |
| const year = parseInt(v[1]); | |
| if (isNaN(year) || year < 1900 || year > 2100) { | |
| return true; | |
| } | |
| return false; | |
| } |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing input validation for the op parameter. The validation only checks if op exists and is "AND" or "OR", but doesn't validate the case sensitivity. Consider normalizing the case (e.g., converting to uppercase) before validation, or document that the parameter is case-sensitive.
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Missing documentation for complex filter logic. The function handles three different filtering strategies (general, roles, cohorts) with different query patterns. Add JSDoc comments explaining the expected input format, behavior of mini_op vs global op, and provide examples of valid filter inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cohortRows should be roleRows instead, because it contains row data.
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N+1 query problem: When mini_op is "AND" for roles, a separate database query is executed for each value in the array (lines 114-124). For large value arrays, this creates a performance bottleneck. Consider refactoring to use a single query with aggregation/grouping to identify volunteers who have all specified roles.
| const roleVolunteerIds: Set<number>[] = []; | |
| for (const v of values) { | |
| const query = client | |
| .from("VolunteerRoles") | |
| .select("volunteer_id, Roles!inner(name)"); | |
| const { data: cohortRows, error } = await query.eq("Roles.name", v); | |
| if (error) return { error: error.message }; | |
| roleVolunteerIds.push(new Set(cohortRows.map((r) => r.volunteer_id))); | |
| } | |
| if (roleVolunteerIds.length > 0) { | |
| volunteerIds = [ | |
| ...roleVolunteerIds.reduce((acc, cur) => { | |
| return acc.intersection(cur); | |
| }), | |
| ]; | |
| } | |
| // Fetch all volunteer-role pairs for the specified roles in a single query | |
| const { data: roleRows, error } = await client | |
| .from("VolunteerRoles") | |
| .select("volunteer_id, Roles!inner(name)") | |
| .in("Roles.name", values); | |
| if (error) return { error: error.message }; | |
| // Group by volunteer_id and count the number of matching roles | |
| const volunteerRoleCount: Record<number, number> = {}; | |
| for (const row of roleRows) { | |
| if (volunteerRoleCount[row.volunteer_id]) { | |
| volunteerRoleCount[row.volunteer_id]++; | |
| } else { | |
| volunteerRoleCount[row.volunteer_id] = 1; | |
| } | |
| } | |
| // Only include volunteers who have all specified roles | |
| volunteerIds = Object.entries(volunteerRoleCount) | |
| .filter(([_, count]) => count === values.length) | |
| .map(([volunteer_id, _]) => Number(volunteer_id)); |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential SQL injection vulnerability in the cohort OR statement construction. The values from user input (v[0] and v[1]) are directly interpolated into the query string without proper escaping. While validation exists, this pattern is risky. Consider using Supabase's query builder methods more safely, or ensure values are properly sanitized.
| let query = client | |
| .from("VolunteerCohorts") | |
| .select("volunteer_id, Cohorts!inner(term, year)"); | |
| const orStatement = values | |
| .map((v) => `and(term.eq.${v[0]},year.eq.${v[1]})`) | |
| .join(","); | |
| query = query.or(orStatement, { referencedTable: "Cohorts" }); | |
| const { data: cohortRows, error } = await query; | |
| if (error) return { error: error.message }; | |
| volunteerIds = cohortRows.map((r) => r.volunteer_id); | |
| // Safely query for each cohort value and union the results | |
| const cohortVolunteerIds: Set<number>[] = []; | |
| for (const v of values) { | |
| const query = client | |
| .from("VolunteerCohorts") | |
| .select("volunteer_id, Cohorts!inner(term, year)") | |
| .eq("Cohorts.term", v[0]) | |
| .eq("Cohorts.year", parseInt(v[1])); | |
| const { data: cohortRows, error } = await query; | |
| if (error) return { error: error.message }; | |
| cohortVolunteerIds.push( | |
| new Set(cohortRows.map((r) => r.volunteer_id)) | |
| ); | |
| } | |
| // Union all sets | |
| if (cohortVolunteerIds.length > 0) { | |
| volunteerIds = [ | |
| ...cohortVolunteerIds.reduce((acc, cur) => { | |
| return new Set([...acc, ...cur]); | |
| }), | |
| ]; | |
| } |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Code duplication in role and cohort filtering logic. Both the role filtering (lines 112-132) and cohort filtering (lines 151-175) blocks implement nearly identical AND operation logic. Consider extracting this into a reusable helper function to improve maintainability.
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N+1 query problem: When mini_op is "AND" for cohorts, a separate database query is executed for each value in the array (lines 153-167). For large value arrays, this creates a performance bottleneck. Consider refactoring to use a single query with aggregation/grouping to identify volunteers who are in all specified cohorts.
| const cohortVolunteerIds: Set<number>[] = []; | |
| for (const v of values) { | |
| const query = client | |
| .from("VolunteerCohorts") | |
| .select("volunteer_id, Cohorts!inner(term, year)"); | |
| const { data: cohortRows, error } = await query | |
| .eq("Cohorts.term", v[0]) | |
| .eq("Cohorts.year", parseInt(v[1])); | |
| if (error) return { error: error.message }; | |
| cohortVolunteerIds.push( | |
| new Set(cohortRows.map((r) => r.volunteer_id)) | |
| ); | |
| } | |
| if (cohortVolunteerIds.length > 0) { | |
| volunteerIds = [ | |
| ...cohortVolunteerIds.reduce((acc, cur) => { | |
| return acc.intersection(cur); | |
| }), | |
| ]; | |
| } | |
| // Refactored: Single query for all cohort values, then group by volunteer_id | |
| // Build filter for all cohort values | |
| const orStatement = values | |
| .map((v) => `and(term.eq.${v[0]},year.eq.${v[1]})`) | |
| .join(","); | |
| let query = client | |
| .from("VolunteerCohorts") | |
| .select("volunteer_id, Cohorts!inner(term, year)"); | |
| query = query.or(orStatement, { referencedTable: "Cohorts" }); | |
| const { data: cohortRows, error } = await query; | |
| if (error) return { error: error.message }; | |
| // Group by volunteer_id and count matches | |
| const volunteerCount: Record<number, number> = {}; | |
| for (const row of cohortRows) { | |
| volunteerCount[row.volunteer_id] = (volunteerCount[row.volunteer_id] || 0) + 1; | |
| } | |
| // Only volunteers present in all cohorts | |
| volunteerIds = Object.entries(volunteerCount) | |
| .filter(([_, count]) => count === values.length) | |
| .map(([volunteer_id, _]) => Number(volunteer_id)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Copilot, AND is not correct here, the loop you are doing will act as: query.eq(field, a).eq(field, b).eq(field, c)... which is incorrect.
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AND operation for general volunteer filtering is incorrect. Chaining multiple .eq() calls on the same field will result in only the last condition being applied, not an AND of all values. This logic doesn't make semantic sense either - a field can't equal multiple different values simultaneously. Consider if this should be OR operation instead, or clarify the intended behavior.
| values.forEach((v) => { | |
| query = query.eq(filterField, v); | |
| }); | |
| } | |
| const { data: volunteerRows, error } = await query; | |
| if (error) return { error: error.message }; | |
| volunteerIds = volunteerRows.map((r) => r.id); | |
| // For AND, get volunteers matching each value, then intersect | |
| const valueVolunteerIds: Set<number>[] = []; | |
| for (const v of values) { | |
| const { data: volunteerRows, error } = await client | |
| .from("Volunteers") | |
| .select("id") | |
| .eq(filterField, v); | |
| if (error) return { error: error.message }; | |
| valueVolunteerIds.push(new Set(volunteerRows.map((r) => r.id))); | |
| } | |
| if (valueVolunteerIds.length > 0) { | |
| volunteerIds = [ | |
| ...valueVolunteerIds.reduce((acc, cur) => { | |
| return acc.intersection(cur); | |
| }), | |
| ]; | |
| } | |
| } | |
| // Only needed for OR branch | |
| if (mini_op === "OR") { | |
| const { data: volunteerRows, error } = await query; | |
| if (error) return { error: error.message }; | |
| volunteerIds = volunteerRows.map((r) => r.id); | |
| } |
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The function can return early with an empty array (line 200) when querySets.length === 0, but this scenario should be impossible given the code flow. If filtersList.length === 0, the function returns early at line 90. Otherwise, at least one set is added to querySets. This check appears to be dead code and should be removed, or clarify if there's an edge case being handled.
| if (querySets.length === 0) return { data: [] }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| // Add any exports from your API files here | ||
|
|
||
| export { getExample } from "./getExample"; | ||
| export { filterMultipleColumns, validateFilter } from "./filterMultipleColumns"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to implement
route.tsfile.