feat: change validator #2692
feat: change validator #2692
Conversation
WalkthroughThis pull request includes several significant changes across multiple files, primarily focusing on the introduction of new data structures and the removal of existing interfaces and schemas related to application management in the CEE and APDF services. Key modifications involve the re-exporting of functionalities from the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (19)
api/src/pdc/services/acquisition/dto/StatusJourney.ts (2)
1-9: Add JSDoc documentation for better maintainabilityConsider adding documentation to explain the purpose and usage of this DTO, especially since it's a new file that other developers will need to understand.
+/** + * Represents the structure for journey status updates. + * Used to validate incoming journey status data from operators. + * + * @example + * { + * operator_id: 123, + * operator_journey_id: "journey_123" + * } + */ export const StatusJourney = object({ operator_id: Serial, operator_journey_id: OperatorJourneyId, }); +/** Type definition for journey status data */ export type StatusJourney = Infer<typeof StatusJourney>;
4-7: Consider adding additional validation constraintsThe schema could benefit from more specific validation rules to ensure data integrity. Consider adding constraints like:
- Non-empty checks for
operator_journey_id- Format validation for
operator_journey_idif it follows a specific patternWould you like me to provide an example implementation with additional validation rules?
api/src/pdc/services/acquisition/dto/CancelJourney.ts (1)
3-4: Add JSDoc comments and consider validation edge cases
- Consider adding JSDoc comments to document the purpose and constraints of these validation rules.
- The
CancelCoderegex allows empty strings ({0,32}). Consider if this is intentional or if it should require at least one character ({1,32}).+/** Maximum 512 character message describing the cancellation reason */ const CancelMessage = size(string(), 0, 512); +/** Alphanumeric code (with underscores/hyphens) identifying the cancellation reason */ -const CancelCode = pattern(string(), /^[0-9A-Za-z_-]{0,32}$/); +const CancelCode = pattern(string(), /^[0-9A-Za-z_-]{1,32}$/);api/src/pdc/services/acquisition/dto/PatchJourney.ts (1)
12-22: Consider implementing comprehensive validation strategyWhile the schema provides basic structural validation, consider implementing:
- Cross-field validation rules (e.g., start/end time relationship)
- Business rule validations (e.g., maximum allowed distance)
- Rate limiting or size restrictions for the entire payload
These could be implemented either:
- As custom validators in the schema
- In a separate validation layer
- Using middleware for cross-cutting concerns
api/src/pdc/providers/superstruct/ValidatorMiddleware.ts (1)
22-32: Consider adding validation failure logging.For better observability and debugging, consider adding logging of validation failures before throwing the InvalidParamsException. This would help with monitoring and troubleshooting validation issues in production.
} catch (e) { if (e instanceof superstruct.StructError) { + const failures = e.failures().map((f) => `${f.path} : ${f.message}`); + console.warn('Validation failed:', { failures }); - throw new InvalidParamsException(e.failures().map((f) => `${f.path} : ${f.message}`)); + throw new InvalidParamsException(failures); } throw new UnexpectedException(`Validator has not been properly configured : ${JSON.stringify(schema)}`); }api/src/pdc/services/acquisition/actions/CancelJourneyAction.ts (1)
28-32: Document the api_version fallbackConsider adding a comment explaining why version "3" is used as the default API version. This would help maintain the code's clarity for future developers.
+ // Default to API version 3 when version range is not specified in the context api_version: context.call?.api_version_range || "3",api/src/pdc/services/acquisition/actions/PatchJourneyAction.ts (1)
40-45: Consider making API version configurable.The hardcoded fallback to api_version "3" could cause issues if API versioning changes. Consider making this configurable or documenting why version "3" is the default.
- api_version: context.call?.api_version_range || "3", + api_version: context.call?.api_version_range || process.env.DEFAULT_API_VERSION || "3",api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts (1)
7-7: LGTM! Clean transition to middleware-based validationThe shift from extension-based validation to middleware-based validation using ValidatorMiddleware is well-structured and maintains clean architecture principles.
Also applies to: 18-39
api/src/pdc/services/acquisition/dto/CreateJourney.ts (2)
50-55: Consider using an enum for payment typeThe
typefield is currently defined asVarchar, which allows any string value. Consider using an enum to restrict possible payment types for better type safety and validation.- type: Varchar, + type: enums(["cash", "card", "transfer"]), // Add appropriate payment types
1-79: Add JSDoc comments for validation rulesConsider adding JSDoc comments to document:
- The purpose of each schema
- The reasoning behind size constraints (e.g., why 20 payments maximum?)
- The validation rules that apply to each field
This will help other developers understand the business rules encoded in these schemas.
api/src/pdc/providers/carpool/interfaces/database/carpool.ts (1)
26-40: Review PII handling and data protection complianceThe interface handles sensitive personal information (phone numbers, travel pass IDs, license plates). While making these fields optional improves flexibility, ensure:
- Implement proper data encryption for stored PII
- Add field-level access controls
- Consider implementing data retention policies
- Document the PII fields and their handling requirements
Would you like me to provide a template for PII handling documentation?
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (2)
11-17: Consider making the anomaly label type more flexibleThe
AnomalyErrorDetailsinterface has a hardcoded label value. Consider using a union type or enum to allow for future anomaly types.- label: "temporal_overlap_anomaly"; + label: AnomalyLabel;And define the type:
type AnomalyLabel = "temporal_overlap_anomaly" | "other_anomaly_types";
Line range hint
47-49: Consider making the API version configurableThe hardcoded API version fallback "3.1" should be configurable to support future API versions.
Consider:
- Moving this to a configuration constant
- Adding version validation
api/src/pdc/services/acquisition/dto/shared.ts (2)
26-29: Consider using a simpler email validation patternThe current email regex is complex and might miss some valid email addresses while potentially allowing invalid ones. Consider using a simpler pattern or a dedicated email validation library.
- /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/, + /^[^@\s]+@[^@\s]+\.[^@\s]+$/,
1-76: Consider grouping related validatorsTo improve maintainability as the file grows, consider organizing related validators into namespaces or separate files:
- Geographic validators (Lat, Lon)
- Identity-related validators (IdentityKey, Email, Phone)
- Journey-related validators (JourneyStatus, OperatorClass)
- Pagination validators (Limit, Offset)
This would make the code more modular and easier to maintain.
api/src/pdc/services/acquisition/actions/ListJourneyAction.ts (1)
Line range hint
37-52: Consider extracting date validation logic and configurationThe method handles multiple responsibilities. Consider:
- Extracting date validation logic to a separate service
- Moving the 90-day limit to configuration
Here's a suggested refactor:
// In a separate config file export const DATE_CONSTRAINTS = { MAX_DAYS_IN_PAST: 90, DEFAULT_DAYS_RANGE: 7, }; // In a separate date validation service export class JourneyDateValidator { public validate(startDate: Date, endDate: Date, todayDate: Date): void { this.validateDateOrder(startDate, endDate); this.validateFutureDate(endDate, todayDate); this.validatePastLimit(startDate, todayDate); } private validateDateOrder(start: Date, end: Date): void { if (isAfter(start, end)) { throw new InvalidParamsException("Start should be before end"); } } // ... other validation methods }api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts (3)
Line range hint
51-51: Remove type assertion to 'any'The type assertion to
anyforoperator_classbypasses TypeScript's type checking. Consider defining proper type for operator_class.- operator_class: params.operator_class as any, + operator_class: params.operator_class,
Line range hint
85-98: Enhance error handling and loggingThe current error handling could be improved in several ways:
- Consider logging the full error object to preserve stack traces
- Add more context to error logs (e.g., journey details)
- Consider categorizing errors for better error reporting
} catch (e) { - logger.error(e.message, { operator_journey_id: params.operator_journey_id, operator_id: params.operator_id }); + logger.error('Journey creation failed', { + error: e, + operator_journey_id: params.operator_journey_id, + operator_id: params.operator_id, + start_datetime: params.start.datetime, + end_datetime: params.end.datetime + }); throw e; }
Line range hint
102-111: Consider enhancing journey validationThe current validation only checks datetime constraints. Consider adding validations for:
- Coordinate bounds (valid lat/lon ranges)
- Reasonable distance values
- Required fields presence
protected async validate( journey: CreateJourney, ): Promise<void> { const now = new Date(); const start = journey.start.datetime; const end = journey.end.datetime; + + // Validate coordinates + if (journey.start.lat < -90 || journey.start.lat > 90 || + journey.end.lat < -90 || journey.end.lat > 90 || + journey.start.lon < -180 || journey.start.lon > 180 || + journey.end.lon < -180 || journey.end.lon > 180) { + throw new ParseErrorException("Invalid coordinates"); + } + + // Validate reasonable distance + if (journey.distance <= 0 || journey.distance > 1000) { + throw new ParseErrorException("Journey distance out of reasonable bounds"); + } + if (end > now || start > end) { throw new ParseErrorException("Journeys cannot happen in the future"); } return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (34)
api/src/lib/superstruct/index.ts(1 hunks)api/src/pdc/providers/carpool/interfaces/database/carpool.ts(1 hunks)api/src/pdc/providers/superstruct/ValidatorMiddleware.ts(2 hunks)api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts(2 hunks)api/src/pdc/services/acquisition/actions/CancelJourneyAction.ts(2 hunks)api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts(3 hunks)api/src/pdc/services/acquisition/actions/ListJourneyAction.ts(1 hunks)api/src/pdc/services/acquisition/actions/PatchJourneyAction.ts(2 hunks)api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts(2 hunks)api/src/pdc/services/acquisition/config/ajv.ts(0 hunks)api/src/pdc/services/acquisition/config/callContext.ts(0 hunks)api/src/pdc/services/acquisition/config/index.ts(0 hunks)api/src/pdc/services/acquisition/contracts/cancel.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/cancel.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionErrorInterface.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionInterface.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/interfaces/CreateJourneyDTO.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/schemas/createJourneyCommon.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/schemas/createJourneySchemaV3.ts(0 hunks)api/src/pdc/services/acquisition/contracts/create.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/create.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/list.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/list.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/patch.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/patch.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/process.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/status.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/status.schema.ts(0 hunks)api/src/pdc/services/acquisition/dto/CancelJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/CreateJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/ListJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/PatchJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/StatusJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/shared.ts(1 hunks)
💤 Files with no reviewable changes (19)
- api/src/pdc/services/acquisition/config/ajv.ts
- api/src/pdc/services/acquisition/config/callContext.ts
- api/src/pdc/services/acquisition/config/index.ts
- api/src/pdc/services/acquisition/contracts/cancel.contract.ts
- api/src/pdc/services/acquisition/contracts/cancel.schema.ts
- api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionErrorInterface.ts
- api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionInterface.ts
- api/src/pdc/services/acquisition/contracts/common/interfaces/CreateJourneyDTO.ts
- api/src/pdc/services/acquisition/contracts/common/schemas/createJourneyCommon.ts
- api/src/pdc/services/acquisition/contracts/common/schemas/createJourneySchemaV3.ts
- api/src/pdc/services/acquisition/contracts/create.contract.ts
- api/src/pdc/services/acquisition/contracts/create.schema.ts
- api/src/pdc/services/acquisition/contracts/list.contract.ts
- api/src/pdc/services/acquisition/contracts/list.schema.ts
- api/src/pdc/services/acquisition/contracts/patch.contract.ts
- api/src/pdc/services/acquisition/contracts/patch.schema.ts
- api/src/pdc/services/acquisition/contracts/process.contract.ts
- api/src/pdc/services/acquisition/contracts/status.contract.ts
- api/src/pdc/services/acquisition/contracts/status.schema.ts
✅ Files skipped from review due to trivial changes (2)
- api/src/lib/superstruct/index.ts
- api/src/pdc/services/acquisition/dto/ListJourney.ts
🔇 Additional comments (31)
api/src/pdc/services/acquisition/dto/StatusJourney.ts (1)
1-2: LGTM: Clean and focused imports
The imports are well-organized and properly scoped, importing only the necessary utilities and types.
api/src/pdc/services/acquisition/dto/CancelJourney.ts (3)
1-2: Verify the import path alias resolution
The @/ alias in the import path needs to be properly configured in the TypeScript/build configuration to resolve correctly.
6-11: LGTM! Well-structured schema definition
The object schema is well-defined with appropriate types and validations. The field names follow a consistent naming convention, and the optional message field provides flexibility.
12-12: LGTM! Clean type export
The type export follows the recommended pattern for superstruct, providing both the validation object and its inferred type.
api/src/pdc/services/acquisition/dto/PatchJourney.ts (4)
1-10: LGTM! Well-organized imports with clear separation of concerns.
The imports are properly structured and all imported entities are utilized in the schema definition.
17-18: Verify temporal validation for start and end points
Ensure that the TimeGeoPoint type includes validation to guarantee that:
- The end time is after the start time
- The timestamps are within reasonable bounds
24-24: LGTM! Proper type inference from schema.
The type export correctly uses superstruct's Infer utility to maintain type safety.
21-21:
Consider adding type constraints for incentive_counterparts
Using unknown() type provides no validation guarantees and could potentially allow malicious data through. Consider defining a more specific structure or adding validation rules if the structure is known.
api/src/pdc/providers/superstruct/ValidatorMiddleware.ts (2)
11-11: LGTM!
Clean import of the new validate function from the internal superstruct library.
22-26: Verify the impact of coercion on data types.
The new validation logic with coerce: true looks good, but please ensure that all schemas are designed to handle type coercion appropriately, as it could lead to subtle bugs if not carefully managed.
api/src/pdc/services/acquisition/actions/CancelJourneyAction.ts (3)
6-6: LGTM: Clean import of the new DTO
The import of the CancelJourney DTO aligns with the migration from alias-based validation to DTO-based validation.
9-10: Verify API contract compatibility
The handler configuration is now more explicit and the validation middleware has been updated to use the CancelJourney DTO. While these changes improve type safety and validation, please ensure:
- The service name "acquisition" and method "cancel" match the API contract
- Existing clients are compatible with the new DTO validation
Also applies to: 15-15
25-25: Verify the removal of return value
The method signature change from Promise<ResultInterface> to Promise<void> suggests this is now a fire-and-forget operation. Please verify:
- Clients don't expect a response payload
- Error handling is properly implemented in the calling code
api/src/pdc/services/acquisition/actions/PatchJourneyAction.ts (2)
9-15: LGTM! Verify permission scope.
The middleware changes improve type safety and permission handling. The permission scope "operator.acquisition.create" seems appropriate for a patch operation, but please verify this is the intended scope.
29-38: LGTM! Clean transformation logic.
The data transformation is well-structured with proper type safety and immutable operations. The conditional operator_class handling is particularly well done.
api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts (1)
Line range hint 1-40: Verify validation coverage after validator changes
Since this represents a significant change in validation strategy, please ensure:
- All previous validation rules are now handled by the ValidatorMiddleware
- The new DTO-based validation (mentioned in the summary) covers all necessary use cases
- No validation gaps were introduced during the transition
api/src/pdc/services/acquisition/dto/CreateJourney.ts (3)
35-36: Consider consolidating phone fields
The presence of both phone and phone_trunc fields seems redundant. Consider either documenting the difference between these fields or consolidating them if they serve the same purpose.
61-61: Verify seat count limitations
Please confirm that the maximum seat count of 8 aligns with business requirements and vehicle capacity regulations.
76-76: Clarify the type of incentive_counterparts
The use of unknown() type suggests this field's structure is not yet defined. Consider:
- Defining a proper type if the structure is known
- Adding documentation explaining why this field is typed as unknown
- Adding a TODO comment if this is temporary
api/src/pdc/providers/carpool/interfaces/database/carpool.ts (3)
29-32: Verify the impact of optional contact information on business requirements
Making contact information fields optional (driver_phone, driver_phone_trunc, passenger_phone, passenger_phone_trunc, *_travelpass_*) could affect:
- User communication capabilities
- Emergency contact scenarios
- Regulatory compliance requirements
Please confirm:
- Are there alternative mandatory contact methods?
- Does this align with the minimum contact requirements for carpooling services?
- Are there any regulatory requirements mandating contact information storage?
Also applies to: 36-39
26-26: Verify vehicle tracking capabilities with optional license plate
Making licence_plate optional could impact:
- Vehicle tracking
- Safety verification
- Insurance compliance
Please confirm if there are alternative mandatory vehicle identification methods in place.
40-40:
Consider implications of optional age verification
Making passenger_over_18 optional might have safety and legal implications.
Please ensure:
- Age verification is handled elsewhere in the system
- This change complies with legal requirements for minor passenger transport
- The operator has alternative means to verify passenger age
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (1)
28-34: LGTM! Handler configuration is well-structured
The handler configuration correctly specifies the service, method, and includes proper validation middleware.
api/src/pdc/services/acquisition/dto/shared.ts (2)
1-14: LGTM! Well-organized imports
The imports are specific and well-structured, importing only the necessary validators from the local superstruct library.
75-76: LGTM! Appropriate pagination constraints
The pagination limits are well-defined with reasonable constraints that will help prevent performance issues with large result sets.
api/src/pdc/services/acquisition/actions/ListJourneyAction.ts (4)
11-13: Verify if the minimal ResultInterface is sufficient for downstream consumers
The interface only includes operator_journey_id. Consider whether additional journey details might be needed by consumers of this API.
16-23: LGTM! Handler configuration is properly set up
The service, method, and middleware configuration are correctly defined with appropriate permissions and validation setup.
30-34: LGTM! Clean implementation with proper type usage
The handle method follows single responsibility principle and correctly uses the new ListJourney type.
Line range hint 1-70: Verify the impact of migrating from ParamsInterface to ListJourney
While the changes look good, please ensure that:
- All consumers of this API are updated to use the new ListJourney type
- The validation rules in ListJourney DTO match or improve upon the previous ParamsInterface validation
api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts (2)
14-19: LGTM! Clean interface definition with proper typing
The new ResultInterface clearly defines the expected response structure with appropriate types.
22-29: LGTM! Well-structured handler configuration
The handler configuration properly defines the service context and includes appropriate middleware for both permissions and validation.
| @@ -23,7 +41,7 @@ export class StatusJourneyAction extends AbstractAction { | |||
| super(); | |||
| } | |||
|
|
|||
| protected async handle(params: ParamsInterface, context: ContextType): Promise<ResultInterface> { | |||
| protected async handle(params: StatusJourney, context: ContextType): Promise<ResultInterface> { | |||
There was a problem hiding this comment.
Remove 'any' type assertions and improve type safety
There are multiple uses of 'any' type assertions which bypass TypeScript's type checking. This could lead to runtime errors.
Consider:
- Creating proper type definitions for the status and anomaly types
- Using type guards instead of type assertions
type StatusType = Infer<typeof JourneyStatus>;
// Then use:
const status: StatusType = castToStatusEnum(result.status);Also applies to: 57-57, 60-60
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (49)
api/src/pdc/services/cee/dto/DeleteApplication.ts (1)
4-7: Consider adding safety constraints for deletion operations.While the schema is clean and focused, deletion operations typically benefit from additional safety constraints. Consider adding:
- A confirmation field
- A reason field for tracking purposes
- Soft deletion flag
Example enhancement:
export const DeleteApplication = object({ uuid: Uuid, + confirmation: literal(true), + reason: string(), + soft_delete: optional(boolean()), });api/src/pdc/services/apdf/dto/ListApdf.ts (2)
1-2: Consider documenting the purpose of this DTO.Adding JSDoc comments would help other developers understand:
- What APDF stands for
- The purpose of this listing DTO
- When and where this schema should be used
Example documentation:
+/** + * Data Transfer Object for listing APDF (Attestation Preuve De Covoiturage) records. + * Used to validate request parameters when querying APDF listings. + */ import { Infer, object, optional } from "@/lib/superstruct/index.ts"; import { Serial } from "@/pdc/providers/superstruct/shared/index.ts";
3-7: Consider adding additional validation constraints.While the current schema correctly validates the types, you might want to add more specific validation rules:
- Ensure IDs are positive numbers
- Add custom error messages for validation failures
Example enhancement:
export const ListApdf = object({ - campaign_id: Serial, - operator_id: optional(Serial), - territory_id: optional(Serial), + campaign_id: Serial.withMessage('campaign_id must be a valid positive integer'), + operator_id: optional(Serial.withMessage('operator_id must be a valid positive integer')), + territory_id: optional(Serial.withMessage('territory_id must be a valid positive integer')), });api/src/pdc/services/application/dto/ListApplication.ts (2)
1-2: Consider organizing imports alphabeticallyThe imports look good and follow the project's import pattern using the
@/alias. However, for better maintainability, consider organizing them alphabetically.-import { defaulted, enums, Infer, object } from "@/lib/superstruct/index.ts"; -import { Serial } from "@/pdc/providers/superstruct/shared/index.ts"; +import { defaulted, enums, Infer, object } from "@/lib/superstruct/index.ts"; +import { Serial } from "@/pdc/providers/superstruct/shared/index.ts";
1-8: Consider adding JSDoc documentationSince this DTO is part of your application service's public interface, consider adding JSDoc documentation to:
- Describe the purpose of this DTO
- Document each field's purpose and constraints
- Provide usage examples
This will improve maintainability and help other developers understand how to use this DTO correctly.
Example documentation:
/** * Data Transfer Object for listing applications. * Used in the application service to filter and retrieve applications. * * @example * const params: ListApplication = { * owner_id: 123, * owner_service: "operator" * }; */ export const ListApplication = object({ /** Unique identifier of the owner */ owner_id: Serial, /** Service type of the owner, defaults to "operator" */ owner_service: defaulted(enums(["operator"]), "operator"), });api/src/pdc/services/application/dto/RevokeApplication.ts (2)
3-7: Add JSDoc documentation for the schemaConsider adding JSDoc documentation to describe the purpose of each field and any validation constraints.
+/** + * Schema for revoking an application + * @property {number} owner_id - The ID of the application owner + * @property {"operator"} owner_service - The service type, defaults to "operator" + * @property {number} uuid - The unique identifier of the application + */ export const RevokeApplication = object({
3-7: Consider enhancing validation and error handlingThe current schema provides basic type validation, but consider adding:
- Custom error messages for validation failures
- Additional validation rules (e.g., format checks for IDs)
- Transformation rules if needed (e.g., string to number conversion)
This would improve the API's user experience by providing more meaningful error messages.
Example enhancement:
import { define } from "@/lib/superstruct/index.ts"; const SerialWithMessage = define('Serial', Serial, { message: (value) => `Invalid ID format: ${value}` }); export const RevokeApplication = object({ owner_id: SerialWithMessage, owner_service: defaulted(enums(["operator"]), "operator"), uuid: SerialWithMessage, });api/src/pdc/services/application/dto/CreateApplication.ts (2)
4-9: Consider adding validation constraintsThe current schema could benefit from additional validation rules:
export const CreateApplication = object({ - name: Varchar, + name: Varchar.refine((val) => val.length >= 3, "Name must be at least 3 characters"), owner_id: Serial, owner_service: defaulted(enums(["operator"]), "operator"), - permissions: defaulted(array(Varchar), []), + permissions: defaulted(array(Varchar.refine((val) => val.length > 0, "Permission cannot be empty")), []), });
1-11: Consider adding JSDoc documentationThe DTO would benefit from documentation explaining its purpose and field requirements.
+/** + * DTO for creating a new application + * @property {string} name - The name of the application + * @property {number} owner_id - The ID of the application owner + * @property {'operator'} owner_service - The service type of the owner (defaults to 'operator') + * @property {string[]} permissions - List of permissions granted to the application (defaults to empty) + */ export const CreateApplication = object({ name: Varchar, owner_id: Serial, owner_service: defaulted(enums(["operator"]), "operator"), permissions: defaulted(array(Varchar), []), });api/src/pdc/services/cee/dto/shared.ts (2)
1-1: Remove file extension from import pathThe
.tsextension in import paths is not recommended in TypeScript as it can cause issues with different bundlers and module resolution strategies.-import { enums, pattern, string, union } from "@/lib/superstruct/index.ts"; +import { enums, pattern, string, union } from "@/lib/superstruct";
3-4: Add JSDoc comments to document enum purposesThese enums appear to be business-critical constants. Adding documentation would help other developers understand their purpose and valid use cases.
+/** Defines the type of journey for CEE calculations */ export const JourneyType = enums(["short", "long"]); +/** Defines the type of CEE application being processed */ export const CeeApplicationType = enums(["specific", "standardized"]);api/src/pdc/services/apdf/dto/ExportApdf.ts (2)
4-16: Consider adding stricter validation rulesThe structure looks good but could benefit from additional validation:
- Add minimum length validation for
campaign_idarray- Add range validation for timestamps to prevent far future/past dates
- Consider adding timezone format validation
Here's a suggested improvement:
export const ExportApdf = object({ format: optional(object({ - tz: Tz, + tz: Tz, // Consider using a more specific timezone validator })), query: object({ - campaign_id: array(Serial), + campaign_id: array(Serial, { minLength: 1 }), operator_id: optional(array(Serial)), date: optional(object({ - start: Timestamp, - end: Timestamp, + start: Timestamp, // Consider adding range validation + end: Timestamp, // Consider adding range validation })), }), });
1-17: Consider documenting validation strategySince this is part of a larger validation change, consider:
- Adding JSDoc comments to document the validation rules
- Creating a validation strategy document
- Including examples of valid/invalid data structures
This will help maintain consistency across the codebase as more DTOs are added or modified.
api/src/pdc/services/cee/dto/SimulateApplication.ts (2)
1-3: Consider removing.tsextensions from importsTypeScript typically doesn't require
.tsextensions in import statements. These are usually handled by the TypeScript compiler and bundler configurations.-import { Infer, object, optional } from "@/lib/superstruct/index.ts"; -import { IdentityKey, Phone } from "@/pdc/providers/superstruct/shared/index.ts"; -import { DrivingLicenseId, JourneyType, LastNameTrunc } from "./shared.ts"; +import { Infer, object, optional } from "@/lib/superstruct"; +import { IdentityKey, Phone } from "@/pdc/providers/superstruct/shared"; +import { DrivingLicenseId, JourneyType, LastNameTrunc } from "./shared";
1-11: Consider PII handling implicationsThis DTO contains sensitive personal information (phone, driving license, name). Ensure that:
- The truncated versions (
phone_trunc,last_name_trunc) are properly sanitized before reaching this point- Appropriate logging restrictions are in place
- Error messages don't leak sensitive data
- Data is properly encrypted in transit and at rest
api/src/pdc/providers/superstruct/shared/index.ts (3)
5-9: Consider making the Timestamp validator more flexibleThe current timestamp validation is very strict and only accepts a specific ISO 8601 format with millisecond precision and 'Z' timezone. This might be too restrictive for valid use cases.
Consider this more flexible approach:
export const Timestamp = coerce( date(), - pattern(string(), /^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\.[0-9]{3}Z$/), + pattern(string(), /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:?\d{2})?$/), (v) => new Date(v), );This would allow:
- Optional milliseconds with variable precision
- Optional timezone offset in +HH:MM or -HH:MM format
- Z timezone indicator
12-15: Optimize UUID regex patternThe UUID validation pattern uses unnecessary word boundaries (\b).
export const Uuid = pattern( string(), - /^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/, + /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/, );
17-17: Enhance phone number validationThe current phone validation is quite permissive. Consider adding length constraints and more specific formatting rules.
-export const Phone = pattern(Varchar, /^\+[0-9]*$/); +export const Phone = pattern( + size(string(), 8, 15), // Common phone number lengths including country code + /^\+[1-9][0-9]{6,13}$/, // Must start with +, then a non-zero digit, followed by 6-13 digits +);api/src/pdc/services/cee/dto/RegisterApplication.ts (1)
12-12: Consider renaming thedatetimefield to be more specific.The field name
datetimeis too generic and doesn't clearly indicate its purpose in the context of a long journey. Consider using a more descriptive name that indicates what this timestamp represents (e.g.,journey_datetime,departure_time, etc.).- datetime: Timestamp, + journey_datetime: Timestamp,api/src/pdc/services/application/actions/RevokeApplicationAction.ts (2)
22-26: Consider repository abstraction implicationsThe change from
ApplicationRepositoryProviderInterfaceResolvertoApplicationPgRepositoryProvidercouples the action to a specific database implementation. While this might be intentional, consider maintaining the interface-based dependency if database flexibility is desired.
Line range hint
31-34: Consider making owner_service configurableThe hardcoded
owner_service: "operator"might be better passed as a parameter or configured at a higher level for flexibility.- await this.applicationRepository.revoke({ - ...params, - owner_service: "operator", - }); + const config = await this.config.get('application.defaults'); + await this.applicationRepository.revoke({ + ...params, + owner_service: config.owner_service ?? "operator", + });api/src/pdc/services/apdf/APDFServiceProvider.ts (1)
Line range hint
16-29: Architecture looks clean and well-structuredThe service provider configuration follows a clean architecture pattern with clear separation of concerns:
- Storage providers for data persistence
- Action handlers for business logic
- Commands for specific operations
- Middleware chain for cross-cutting concerns
Consider documenting the validation rules and their corresponding actions in the README to help maintain the codebase as it evolves.
api/src/pdc/services/application/actions/ListApplicationAction.ts (1)
23-27: Consider maintaining repository abstractionThe switch to
ApplicationPgRepositoryProvidercouples the action to a PostgreSQL implementation. Consider keeping the repository interface abstract to maintain flexibility for different database implementations.- private applicationRepository: ApplicationPgRepositoryProvider, + private applicationRepository: ApplicationRepositoryInterface,api/src/pdc/services/application/actions/CreateApplicationAction.ts (2)
23-27: Consider using an interface for repository dependencyThe constructor now depends on a concrete PostgreSQL implementation (
ApplicationPgRepositoryProvider) rather than an interface. This tight coupling might make the code less flexible and harder to test.Consider using an interface instead:
- private applicationRepository: ApplicationPgRepositoryProvider, + private applicationRepository: ApplicationRepositoryInterface,
Line range hint
29-37: Consider making default values configurableThe handle method implementation is clean, but it hardcodes default values for
owner_serviceandpermissions. Consider making these configurable through the service configuration or environment variables for better flexibility.Example approach:
+ constructor( + private applicationRepository: ApplicationPgRepositoryProvider, + @inject('config') private config: { defaultOwnerService: string }, + ) { + super(); + } public async handle( params: CreateApplication, ): Promise<ApplicationInterface> { return this.applicationRepository.create({ ...params, - owner_service: "operator", + owner_service: this.config.defaultOwnerService, permissions: [], }); }api/src/pdc/services/acquisition/dto/shared.ts (5)
22-25: Consider simplifying the email regex patternThe current email regex is quite complex. Consider using a simpler pattern that covers most use cases while remaining maintainable:
- /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/, + /^[^@\s]+@[^@\s]+\.[^@\s]+$/,
29-29: Document OperatorClass valuesPlease add JSDoc comments explaining what A, B, and C operator classes represent for better maintainability.
+/** + * Operator classification: + * A - [description] + * B - [description] + * C - [description] + */ export const OperatorClass = enums(["A", "B", "C"]);
31-38: Please clarify the Distance unit in commentsThe structure looks good, but adding documentation about the expected unit (meters?) would improve clarity.
+/** Distance in meters */ export const Distance = size(integer(), 0, 1_000_000);
46-46: Document the purpose of phone_trunc fieldPlease add documentation explaining the difference between
phoneandphone_truncfields and their specific uses.
64-68: Document the significance of index limitPlease explain why the index is limited to 19. Is this a business rule or technical constraint?
api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts (1)
Line range hint
24-41: Consider introducing a base interface for common fieldsBoth
CeeShortApplicationInterfaceandCeeLongApplicationInterfaceshare common fields. Consider extracting these into a base interface to improve maintainability and reduce duplication.Example refactor:
interface BaseCeeApplicationInterface { application_timestamp: Date; last_name_trunc: LastNameTrunc; driving_license: DrivingLicense; identity_key?: string; } interface CeeShortApplicationInterface extends BaseCeeApplicationInterface { journey_type: CeeJourneyTypeEnum.Short; operator_journey_id: OperatorJourneyId; } interface CeeLongApplicationInterface extends BaseCeeApplicationInterface { journey_type: CeeJourneyTypeEnum.Long; phone_trunc: PhoneTrunc; datetime: Date; }api/src/pdc/services/cee/actions/FindCeeAction.ts (1)
27-27: LGTM: Type signatures are more specific and clearThe transition from generic interfaces to specific DTOs improves type safety and code clarity. Consider adding JSDoc comments to document the expected shape of the data and any validation rules.
Add documentation like this:
+ /** + * Find a CEE application by UUID + * @param params - The search parameters containing the UUID + * @param context - The request context containing operator information + * @returns The CEE application details with status and signature + * @throws ForbiddenException if the operator doesn't own the application + */ public async handle( params: FindApplication, context: ContextType, ): Promise<CeeApplicationResultInterface>Also applies to: 29-29
api/src/pdc/services/apdf/actions/ListAction.ts (2)
18-28: Consider enhancing type definitions with constraints and documentation.The type definitions are well-structured, but could benefit from:
- JSDoc comments explaining the purpose of each field
- More specific constraints for fields like
size(non-negative) anddatetime- Optional fields if any of these could be nullable
Consider applying these improvements:
+/** Represents an enriched APDF record with signed URL and metadata */ export type EnrichedApdfType = { + /** Pre-signed URL for accessing the PDF */ signed_url: string; + /** Unique storage key/identifier */ key: string; + /** File size in bytes */ - size: number; + size: number & { __brand: 'NonNegative' }; operator_id: number; campaign_id: number; datetime: Date; name: string; };
Line range hint
57-93: Consider enhancing error handling for repository operations.While the error handling for the campaign lookup is good, consider adding similar error handling for repository operations:
if (!campaign) { throw new NotFoundException(`Could not find campaign ${campaign_id}`); } + try { // curry operators and campaign filters const opsFilter = this.storageRepository.operatorsFilter( operator_id ? [operator_id] : [], ); const cmpFilter = this.storageRepository.campaignsFilter([campaign_id]); const monthFilter = this.storageRepository.showCurrentMonthFilter( context.call!.user.permissions, this.config.get("apdf.showLastMonth"), ); return (await this.storageRepository.enrich( await this.storageRepository.findByCampaign(campaign), )) .filter(cmpFilter) .filter(opsFilter) .filter(monthFilter); + } catch (e) { + logger.error(`[apdf:list -> repository] ${e.message}`); + throw e; + }api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts (4)
Line range hint
23-29: Enhance SQL query security and type safety
- Table name is vulnerable to SQL injection through string interpolation
- Using
anytype for query results loses type safetyConsider these improvements:
- text: ` - SELECT * FROM ${this.table} - WHERE owner_service = $1 - AND owner_id = $2::int - AND deleted_at IS NULL - `, + text: 'SELECT * FROM "application"."applications" WHERE owner_service = $1 AND owner_id = $2::int AND deleted_at IS NULL',Also, consider defining a proper type for the database row instead of using
any:interface ApplicationRow { uuid: string; name: string; owner_id: number; owner_service: string; permissions: string[]; created_at: Date; updated_at: Date; deleted_at: Date | null; }
Line range hint
42-50: Simplify owner_id parameter handlingThe conditional construction of SQL query parts could be simplified using a parameterized approach.
Consider this alternative:
- const ownerParams = owner_id && typeof owner_id !== "string" - ? { - text: "AND owner_id = $3::int", - values: [owner_id], - } - : { - text: "", - values: [], - }; + const values = [owner_service, uuid]; + let text = 'SELECT * FROM "application"."applications" WHERE owner_service = $1 AND uuid = $2'; + + if (owner_id && typeof owner_id === 'number') { + values.push(owner_id); + text += ' AND owner_id = $3::int'; + } + + text += ' AND deleted_at IS NULL LIMIT 1';
Line range hint
65-67: Enhance error handling with NotFoundExceptionThe error handling is inconsistent across methods. The
revokemethod usesNotFoundExceptionwhilefinduses a genericError.Consider using the same error type:
- throw new Error(`Application not found (${uuid})`); + throw new NotFoundException(`Application not found (uuid: ${uuid}, owner_service: ${owner_service})`);
Line range hint
89-91: Improve error handling in create methodThe error message could be more descriptive to aid in debugging.
Consider enhancing the error message:
- throw new Error(`Unable to create application (${name})`); + throw new Error(`Failed to create application (name: ${name}, owner_service: ${owner_service})`);api/src/pdc/services/apdf/actions/ExportAction.ts (1)
Line range hint
123-129: Ensure S3 upload filename is properly sanitizedThe filename is used directly in the S3 upload without explicit sanitization. While the S3 provider might handle this, it's better to be explicit about filename sanitization to prevent potential path traversal issues.
Consider adding filename sanitization:
- const file = await this.s3StorageProvider.upload( - BucketName.APDF, - filepath, - filename, - `${campaign._id}`, - ); + const sanitizedFilename = filename.replace(/[^a-zA-Z0-9._-]/g, '_'); + const file = await this.s3StorageProvider.upload( + BucketName.APDF, + filepath, + sanitizedFilename, + `${campaign._id}`, + );api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts (2)
Line range hint
82-134: Consider using parameterized tests for validation casesThe validation test cases follow a similar pattern and could be refactored using parameterized tests to reduce code duplication and improve maintainability.
Example refactor:
const invalidParams = [ { desc: "invalid last_name_trunc", payload: { last_name_trunc: "abcd" } }, { desc: "unsupported journey type", payload: { journey_type: "bip" } }, // ... other cases ]; invalidParams.forEach(({ desc, payload }) => { it(`Invalid params ${desc}`, async () => { await assertErrorHandler( kernel, handlerConfig, { ...defaultShortPayload, ...payload }, [], defaultContext, ); }); });
Line range hint
164-229: Track and plan removal of deprecated testsTwo test cases are marked with
@deprecated [carpool_v2_migration]. These should be tracked for removal or update as part of the carpool v2 migration.Would you like me to:
- Create an issue to track the removal of deprecated tests?
- Help create migration tests for the new carpool v2 structure?
api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts (3)
Line range hint
252-264: Potential formula injection vulnerability in Excel formulasThe string concatenation in Excel formulas could be vulnerable to injection if the
modeparameter contains special characters. Consider sanitizing the input or using a type-safe approach.- const mode_criteria = `Trajets!S:S,"${mode}"`; + const sanitizedMode = mode.replace(/[^a-zA-Z]/g, ''); + const mode_criteria = `Trajets!S:S,"${sanitizedMode}"`;
Line range hint
313-322: Add input validation for slice valuesThe
formatSliceLabelmethod doesn't validate the slice values before using them. Consider adding validation to prevent potential issues with negative or invalid values.private formatSliceLabel(slice: SliceInterface): string { + if (slice.start < 0 || (slice.end && slice.end < 0)) { + throw new Error('Slice values cannot be negative'); + } + if (slice.start && slice.end && slice.start >= slice.end) { + throw new Error('Start value must be less than end value'); + } if (!slice.start && slice.end) { return `Jusqu'à ${slice.end / 1000} km`; } else if (slice.start && !slice.end) {
Line range hint
313-322: Extract magic number to a named constantThe number 1000 is used for kilometer conversion but its purpose isn't immediately clear. Consider extracting it to a named constant.
+ private static readonly METERS_PER_KM = 1000; + private formatSliceLabel(slice: SliceInterface): string { if (!slice.start && slice.end) { - return `Jusqu'à ${slice.end / 1000} km`; + return `Jusqu'à ${slice.end / SlicesWorksheetWriter.METERS_PER_KM} km`; } else if (slice.start && !slice.end) { - return `Supérieure à ${slice.start / 1000} km`; + return `Supérieure à ${slice.start / SlicesWorksheetWriter.METERS_PER_KM} km`; } - return `De ${slice.start / 1000} km à ${(slice.end || 0) / 1000} km`; + return `De ${slice.start / SlicesWorksheetWriter.METERS_PER_KM} km à ${(slice.end || 0) / SlicesWorksheetWriter.METERS_PER_KM} km`; }api/src/pdc/providers/superstruct/shared/tz.ts (1)
1-2: Consider using a direct import from superstruct.The import is currently going through a re-export layer (
@/lib/superstruct/index.ts). While this works, it might be better to import directly fromsuperstructfor better maintainability and to avoid potential circular dependencies.-import { enums, Infer } from "@/lib/superstruct/index.ts"; +import { enums, Infer } from "superstruct";api/src/pdc/services/cee/actions/RegisterCeeAction.ts (4)
78-79: Use a More Specific Exception for Unsupported Journey TypesThrowing
UnexpectedExceptionfor an unsupportedjourney_typemay obscure the underlying issue. Consider usingInvalidParamsExceptionwith a clear error message to make debugging easier and provide more meaningful feedback.Apply this diff to improve exception handling:
default: - throw new UnexpectedException(); + throw new InvalidParamsException('Unsupported journey_type provided');
99-99: Confirm Necessity of Castingjourney_typeCasting
params.journey_typetoCeeJourneyTypeEnummay be unnecessary ifparams.journey_typeis already validated and typed correctly. Review the type definitions and validations to avoid redundant casting.
118-118: Consider Refactoring to Reduce Code DuplicationBoth
processForShortApplicationandprocessForLongApplicationmethods have similar structures and exception handling logic. Consider refactoring common code into shared helper functions to enhance maintainability and reduce duplication.Also applies to: 185-185
Line range hint
153-169: Handle Potential Server Error ExplicitlyThe exception handling code checks for
!oldand throws a generic error"Boum". Replace this with a more descriptive exception to provide clarity on the server error.Apply this diff to improve error handling:
if (!old) { // Server error - throw new Error("Boum"); + throw new UnexpectedException('Application search returned null'); } else {Also applies to: 220-235
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (69)
api/src/pdc/providers/carpool/repositories/CarpoolStatusRepository.ts(1 hunks)api/src/pdc/providers/superstruct/shared/index.ts(1 hunks)api/src/pdc/providers/superstruct/shared/tz.ts(1 hunks)api/src/pdc/proxy/HttpTransport.ts(5 hunks)api/src/pdc/proxy/middlewares/serverTokenMiddleware.ts(1 hunks)api/src/pdc/services/acquisition/dto/shared.ts(1 hunks)api/src/pdc/services/apdf/APDFServiceProvider.ts(2 hunks)api/src/pdc/services/apdf/actions/ExportAction.ts(2 hunks)api/src/pdc/services/apdf/actions/ListAction.ts(2 hunks)api/src/pdc/services/apdf/contracts/export.contract.ts(0 hunks)api/src/pdc/services/apdf/contracts/export.schema.ts(0 hunks)api/src/pdc/services/apdf/contracts/list.contract.ts(0 hunks)api/src/pdc/services/apdf/contracts/list.schema.ts(0 hunks)api/src/pdc/services/apdf/dto/ExportApdf.ts(1 hunks)api/src/pdc/services/apdf/dto/ListApdf.ts(1 hunks)api/src/pdc/services/apdf/interfaces/APDFRepositoryProviderInterface.ts(1 hunks)api/src/pdc/services/apdf/interfaces/PolicySliceStatInterface.ts(1 hunks)api/src/pdc/services/apdf/providers/APDFRepositoryProvider.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/BuildExcel.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts(1 hunks)api/src/pdc/services/application/ApplicationServiceProvider.ts(2 hunks)api/src/pdc/services/application/actions/CreateApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/FindApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/ListApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/RevokeApplicationAction.ts(1 hunks)api/src/pdc/services/application/contracts/common/interfaces/ApplicationCreateResultInterface.ts(0 hunks)api/src/pdc/services/application/contracts/create.contract.ts(0 hunks)api/src/pdc/services/application/contracts/create.schema.ts(0 hunks)api/src/pdc/services/application/contracts/find.contract.ts(0 hunks)api/src/pdc/services/application/contracts/find.schema.ts(0 hunks)api/src/pdc/services/application/contracts/list.contract.ts(0 hunks)api/src/pdc/services/application/contracts/list.schema.ts(0 hunks)api/src/pdc/services/application/contracts/revoke.contract.ts(0 hunks)api/src/pdc/services/application/contracts/revoke.schema.ts(0 hunks)api/src/pdc/services/application/dto/CreateApplication.ts(1 hunks)api/src/pdc/services/application/dto/FindApplication.ts(1 hunks)api/src/pdc/services/application/dto/ListApplication.ts(1 hunks)api/src/pdc/services/application/dto/RevokeApplication.ts(1 hunks)api/src/pdc/services/application/interfaces/ApplicationRepositoryProviderInterface.ts(0 hunks)api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts(5 hunks)api/src/pdc/services/cee/actions/DeleteCeeAction.ts(2 hunks)api/src/pdc/services/cee/actions/FindCeeAction.ts(2 hunks)api/src/pdc/services/cee/actions/ImportCeeAction.integration.spec.ts(0 hunks)api/src/pdc/services/cee/actions/ImportCeeAction.ts(0 hunks)api/src/pdc/services/cee/actions/ImportCeeIdentityAction.integration.spec.ts(0 hunks)api/src/pdc/services/cee/actions/ImportCeeIdentityAction.ts(0 hunks)api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts(1 hunks)api/src/pdc/services/cee/actions/RegisterCeeAction.ts(7 hunks)api/src/pdc/services/cee/actions/SimulateCeeAction.ts(3 hunks)api/src/pdc/services/cee/contracts/common/ceeSchema.ts(0 hunks)api/src/pdc/services/cee/contracts/deleteApplication.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/deleteApplication.schema.ts(0 hunks)api/src/pdc/services/cee/contracts/findApplication.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/findApplication.schema.ts(0 hunks)api/src/pdc/services/cee/contracts/importApplication.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/importApplication.schema.ts(0 hunks)api/src/pdc/services/cee/contracts/importApplicationIdentity.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/importApplicationIdentity.schema.ts(0 hunks)api/src/pdc/services/cee/contracts/registerApplication.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/registerApplication.schema.ts(0 hunks)api/src/pdc/services/cee/contracts/simulateApplication.contract.ts(0 hunks)api/src/pdc/services/cee/contracts/simulateApplication.schema.ts(0 hunks)api/src/pdc/services/cee/dto/DeleteApplication.ts(1 hunks)api/src/pdc/services/cee/dto/FindApplication.ts(1 hunks)api/src/pdc/services/cee/dto/RegisterApplication.ts(1 hunks)api/src/pdc/services/cee/dto/SimulateApplication.ts(1 hunks)api/src/pdc/services/cee/dto/shared.ts(1 hunks)api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts(2 hunks)api/src/pdc/services/cee/interfaces/CeeRepositoryProviderInterface.ts(2 hunks)
💤 Files with no reviewable changes (31)
- api/src/pdc/services/apdf/contracts/export.contract.ts
- api/src/pdc/services/apdf/contracts/export.schema.ts
- api/src/pdc/services/apdf/contracts/list.contract.ts
- api/src/pdc/services/apdf/contracts/list.schema.ts
- api/src/pdc/services/application/contracts/common/interfaces/ApplicationCreateResultInterface.ts
- api/src/pdc/services/application/contracts/create.contract.ts
- api/src/pdc/services/application/contracts/create.schema.ts
- api/src/pdc/services/application/contracts/find.contract.ts
- api/src/pdc/services/application/contracts/find.schema.ts
- api/src/pdc/services/application/contracts/list.contract.ts
- api/src/pdc/services/application/contracts/list.schema.ts
- api/src/pdc/services/application/contracts/revoke.contract.ts
- api/src/pdc/services/application/contracts/revoke.schema.ts
- api/src/pdc/services/application/interfaces/ApplicationRepositoryProviderInterface.ts
- api/src/pdc/services/cee/actions/ImportCeeAction.integration.spec.ts
- api/src/pdc/services/cee/actions/ImportCeeAction.ts
- api/src/pdc/services/cee/actions/ImportCeeIdentityAction.integration.spec.ts
- api/src/pdc/services/cee/actions/ImportCeeIdentityAction.ts
- api/src/pdc/services/cee/contracts/common/ceeSchema.ts
- api/src/pdc/services/cee/contracts/deleteApplication.contract.ts
- api/src/pdc/services/cee/contracts/deleteApplication.schema.ts
- api/src/pdc/services/cee/contracts/findApplication.contract.ts
- api/src/pdc/services/cee/contracts/findApplication.schema.ts
- api/src/pdc/services/cee/contracts/importApplication.contract.ts
- api/src/pdc/services/cee/contracts/importApplication.schema.ts
- api/src/pdc/services/cee/contracts/importApplicationIdentity.contract.ts
- api/src/pdc/services/cee/contracts/importApplicationIdentity.schema.ts
- api/src/pdc/services/cee/contracts/registerApplication.contract.ts
- api/src/pdc/services/cee/contracts/registerApplication.schema.ts
- api/src/pdc/services/cee/contracts/simulateApplication.contract.ts
- api/src/pdc/services/cee/contracts/simulateApplication.schema.ts
✅ Files skipped from review due to trivial changes (3)
- api/src/pdc/services/apdf/interfaces/APDFRepositoryProviderInterface.ts
- api/src/pdc/services/apdf/interfaces/PolicySliceStatInterface.ts
- api/src/pdc/services/apdf/providers/APDFRepositoryProvider.ts
🔇 Additional comments (77)
api/src/pdc/services/cee/dto/FindApplication.ts (2)
4-7: Consider enhancing the validation schema
The schema only validates the UUID format. Consider whether additional validation rules or fields are needed for the CEE application finding use case.
✅ Verification successful
Let me check the other FindApplication DTO and the actual usage context to make a more informed decision.
The current FindApplication DTO schema is appropriate for its use case
The FindApplication DTO in the CEE service is correctly designed for its specific use case. Looking at the FindCeeAction implementation, the UUID is the only required parameter as it's used to find a specific CEE application, and additional validation is handled within the action itself:
- The operator_id is obtained from the context
- The application ownership is verified against the operator_id
- The response includes specific CEE-related fields like journey_id, status, and a signed token
The other FindApplication DTO you found is for a different service with different requirements, so its schema shouldn't influence this one.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar DTO patterns in the codebase
# Look for similar FindApplication patterns
echo "Checking for similar Find DTOs..."
rg --type ts "export const Find.*= object\(" api/src/pdc/services/
# Look for usage of this DTO
echo "Checking usage of FindApplication..."
rg --type ts "FindApplication" api/src/pdc/services/cee/
Length of output: 970
Script:
#!/bin/bash
# Check the other FindApplication DTO pattern
echo "Checking other FindApplication DTO..."
cat api/src/pdc/services/application/dto/FindApplication.ts
# Check the FindCeeAction implementation to understand usage context
echo -e "\nChecking FindCeeAction implementation..."
cat api/src/pdc/services/cee/actions/FindCeeAction.ts
Length of output: 3038
1-2: Verify import paths
The imports use absolute paths with the @/ prefix. Let's verify these paths exist and are correctly configured.
✅ Verification successful
Import paths are correctly configured and used
The imports are valid and correctly structured:
@/lib/superstruct/index.tsexports all fromjsr:@superstruct/core@^2@/pdc/providers/superstruct/shared/index.tsexists and exports theUuidtype as used in the import- Both files are present in the correct locations under the
api/src/directory - The circular dependency check shows that these imports are properly structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of imported modules and their exports
# Check the superstruct library import
echo "Checking superstruct import..."
fd --type f "index.ts" lib/superstruct/
# Check the Uuid type import
echo "Checking Uuid type import..."
fd --type f "shared/index.ts" "pdc/providers/superstruct"
# Verify the exports
echo "Checking exports in the files..."
rg --type ts "export.*object|export.*Uuid" lib/superstruct/ pdc/providers/superstruct/
Length of output: 738
Script:
#!/bin/bash
# Let's first understand the repository structure by looking for these files anywhere in the codebase
echo "Searching for index.ts files..."
fd -e ts "^index\.ts$"
echo -e "\nSearching for any TypeScript files with 'superstruct' in their path..."
fd -e ts . | grep -i "superstruct"
echo -e "\nSearching for TypeScript files containing imports with '@/'..."
rg --type ts "^import.*from\s+['\"]\@\/" -l
echo -e "\nLooking for tsconfig.json or similar files that might define path aliases..."
fd "tsconfig.*\.json$"
Length of output: 5603
Script:
#!/bin/bash
# Let's verify the content of these specific files and the path alias configuration
# Check the content of the superstruct files we found
echo "Checking lib/superstruct/index.ts content..."
cat api/src/lib/superstruct/index.ts
echo -e "\nChecking pdc/providers/superstruct/shared/index.ts content..."
cat api/src/pdc/providers/superstruct/shared/index.ts
# Check the tsconfig.json in the api directory for path aliases
echo -e "\nChecking for tsconfig.json in api directory..."
fd "tsconfig.*\.json$" api/
# If we find it, let's look at its content
echo -e "\nChecking api tsconfig.json content (if exists)..."
find api/ -name "tsconfig*.json" -exec cat {} \;
Length of output: 1319
api/src/pdc/services/cee/dto/DeleteApplication.ts (2)
1-2: LGTM! Clean and focused imports.
The imports are well-organized, following good practices by importing specific utilities and using aliased paths.
2-2: Verify the Uuid type validation rules.
Let's ensure the imported Uuid type implements proper validation rules for UUID format.
✅ Verification successful
The Uuid type implements proper UUID v4 format validation
The implementation in api/src/pdc/providers/superstruct/shared/index.ts uses a precise regular expression pattern that validates the standard UUID format: 8 hex digits, followed by three groups of 4 hex digits, and ending with 12 hex digits, all separated by hyphens.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of the Uuid type
# Look for the Uuid type definition
ast-grep --pattern 'export const Uuid = $_'
# Check for any existing tests
rg -l "Uuid.*test" -g "*.test.*"
Length of output: 576
api/src/pdc/services/apdf/dto/ListApdf.ts (1)
1-9: LGTM! Clean and well-structured DTO implementation.
The schema definition is clear and follows good practices with proper type inference and optional field handling.
api/src/pdc/services/application/dto/ListApplication.ts (2)
8-8: LGTM! Type export follows best practices
The type export using Infer is the recommended approach when working with superstruct, as it ensures the TypeScript type stays in sync with the runtime validator.
3-6: Verify the completeness of the ListApplication structure
The structure seems minimal with only owner_id and owner_service. Based on the AI summary mentioning status, limit, offset, start, and end fields, it appears some fields might be missing from this implementation.
api/src/pdc/services/application/dto/RevokeApplication.ts (3)
1-2: LGTM! Well-organized imports
The imports are specific and well-structured, using local paths appropriately.
9-9: LGTM! Proper type inference
The type export correctly uses Infer to maintain consistency with the schema definition.
4-4: Verify Serial type implementation for ID fields
The use of Serial type for both owner_id and uuid needs verification to ensure proper validation of numeric identifiers.
Also applies to: 6-6
✅ Verification successful
Let me gather more information about the Serial type implementation and its usage.
Serial type implementation is correctly defined and used
The Serial type is properly implemented as a bounded integer between 0 and 2147483647 (max 32-bit signed integer) using the size and integer validators. Its usage for both owner_id and uuid fields in RevokeApplication is consistent with the type definition and provides proper validation of numeric identifiers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Serial type implementation and its usage
# Check Serial type implementation
ast-grep --pattern 'export const Serial = $_'
# Check for other usages of Serial type in DTOs
rg -l "Serial.*owner_id|uuid.*Serial" --type ts
Length of output: 77372
Script:
#!/bin/bash
# Check the implementation and usage of Serial type in the codebase
# Check the Serial type implementation in the shared types
cat api/src/pdc/providers/superstruct/shared/index.ts
# Check the usage context in the relevant files
cat api/src/pdc/services/application/dto/RevokeApplication.ts
cat api/src/pdc/services/application/dto/FindApplication.ts
Length of output: 1560
api/src/pdc/services/application/dto/FindApplication.ts (2)
9-9: LGTM! Type alias follows best practices
The type alias naming and usage of Infer follows TypeScript best practices for superstruct schema typing.
3-7: Review the schema design choices
There are several potential concerns with the current schema design:
- If
uuidis meant to be a UUID, usingSerialtype might be too restrictive - The mixed type for
owner_id(Serial | Varchar) suggests inconsistent data modeling - The
owner_serviceenum only allows "operator" which seems overly restrictive for future extensibility
Let's verify the usage of these fields across the codebase:
✅ Verification successful
Schema design is appropriate for the current implementation
Based on the codebase analysis:
-
The
uuidasSerialtype is correct as it's consistently used across the application (RevokeApplication, FindApplication) and matches the database schema where it's used as an integer identifier. -
The mixed type for
owner_id(Serial | Varchar) in FindApplication is intentionally designed to support both:- New applications using numeric IDs (Serial)
- Legacy V1 tokens with string IDs (as evidenced in
serverTokenMiddleware.ts: "V1 tokens have a string owner_id")
-
The
owner_serviceenum with only "operator" is appropriate as:- All application-related schemas consistently use only "operator"
- Database queries and application logic are built around this single service type
- The default value ensures backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for UUID usage patterns
rg -t typescript "uuid.*Serial" api/src/pdc/services
# Check for owner_service values
rg -t typescript "owner_service.*=.*['\"](?!operator['\"])" api/src/pdc/services
# Check for owner_id type usage
rg -t typescript "owner_id.*:" api/src/pdc/services
Length of output: 529
Script:
#!/bin/bash
# Check for UUID usage patterns without file type restriction
rg "uuid.*Serial" --type-add 'ts:*.ts' --type ts
# Check for owner_service values
rg "owner_service.*=" --type-add 'ts:*.ts' --type ts
# Check for owner_id type usage and its context
rg "owner_id" --type-add 'ts:*.ts' --type ts -A 2
# Look for other schema definitions to understand the pattern
ast-grep --pattern 'export const $_ = object({
$$$
})'
Length of output: 25953
api/src/pdc/services/application/dto/CreateApplication.ts (3)
1-2: LGTM: Clean and focused imports
The imports are well-organized, using internal library paths and importing specific types needed for the DTO.
11-11: LGTM: Clean type export
The type inference using Infer is a good practice, ensuring type safety and maintaining single source of truth.
4-9: Security consideration: Review default permissions and service
While the implementation is technically correct, there are two security considerations to verify:
- Defaulting
owner_serviceto "operator" could potentially grant elevated privileges if not properly handled downstream - An empty permissions array by default follows the principle of least privilege, but verify if this is the intended behavior
✅ Verification successful
Based on the results, I can now generate a final response as we have sufficient information about how permissions and owner_service are handled in the codebase.
Default permissions and service configuration are properly secured
The codebase shows robust security handling:
- The default "operator" service is properly validated through middleware checks, particularly in
serverTokenMiddleware.tswhich validates owner_service and permissions - Empty permissions array by default is correct as the codebase implements proper permission checks:
- HasPermissionMiddleware enforces explicit permission grants
- Multiple security layers validate permissions at different levels (proxy, middleware)
- ForbiddenException is thrown when required permissions are missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how owner_service and permissions are used in the application flow
# Look for permission checks and service validations
# Search for permission checks
echo "Searching for permission validation logic..."
rg -A 5 "permissions.*check|validatePermissions|checkPermissions"
# Search for owner_service usage
echo "Searching for owner_service handling..."
rg -A 5 "owner_service.*validate|validateService"
# Look for related security middleware
echo "Checking for security middleware..."
fd -e ts -e js middleware | xargs rg "permission|auth|owner"
Length of output: 18379
api/src/pdc/services/cee/dto/shared.ts (2)
6-11: Review and document DrivingLicenseId validation patterns
The current implementation raises several concerns:
- The pattern starting with
99-is too permissive and accepts any characters after the prefix - There's no documentation explaining which formats correspond to which types of driving licenses
- The validation might be too lenient for certain formats
Consider:
- Adding constraints to the
99-pattern - Documenting each format's purpose
- Adding examples of valid/invalid formats
5-5: Document LastNameTrunc pattern requirements
The pattern /^[A-Z ]{3}$/ is very specific (exactly 3 uppercase letters or spaces). Please add documentation explaining:
- Why exactly 3 characters are required
- The business rules behind this truncation
- How to handle names that don't fit this pattern
api/src/pdc/services/apdf/dto/ExportApdf.ts (3)
17-17: LGTM! Good use of type inference
Using Infer to derive the type from the validation schema ensures type safety and maintainability.
1-2: Verify the imported shared types
The imports look correct, but let's verify the shared types being used.
✅ Verification successful
The imported shared types are correctly defined and used
The imports in ExportApdf.ts are valid. The verification shows that:
Serialis defined as a size-constrained integer validator (0 to 2147483647)Timestampis defined as a coerced date validator with ISO string patternTzis defined in a separate file and properly exported, containing a comprehensive list of timezone enums
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the shared types (Serial, Timestamp, Tz) exist and are properly exported
# Check the shared types file
ast-grep --pattern 'export const $type = $_' src/pdc/providers/superstruct/shared/index.ts
Length of output: 185
Script:
#!/bin/bash
# Let's first find the correct location of the shared types
fd -e ts -e js "shared" --type f
# Then let's search for these type definitions across the codebase
rg "export.*(Serial|Timestamp|Tz)\s*=" -g "*.ts" -g "*.js"
# Also search for any imports of these types to understand their location
rg "import.*{.*(?:Serial|Timestamp|Tz).*}" -g "*.ts" -g "*.js"
Length of output: 927
Script:
#!/bin/bash
# Let's verify the exact content of the superstruct shared types
cat api/src/pdc/providers/superstruct/shared/index.ts
# And also check the content of tz.ts since it seems to be related
cat api/src/pdc/providers/superstruct/shared/tz.ts
# Fix the import search with proper escaping
rg "import.*\{[^}]*?(Serial|Timestamp|Tz)[^}]*?\}" -g "*.ts"
Length of output: 17791
11-14: Verify timestamp handling across timezones
The optional date range with timestamps needs careful handling when the timezone is specified in the format.
✅ Verification successful
Let me analyze the timestamp handling in the codebase to verify the concern.
Let me check the implementation of the Timestamp type and date handling.
Based on the findings, I can now provide a final response:
Timestamp handling is properly implemented with timezone support
The codebase shows that:
- The Timestamp type is properly defined using superstruct with ISO-8601 format validation (
/^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\.[0-9]{3}Z$/) - The date handling implementation includes:
- Proper timezone conversion using
fromZonedTime - Consistent use of ISO string format for serialization
- Default timezone handling (Europe/Paris)
- Proper timezone conversion using
- The optional date range in the DTO uses this validated Timestamp type, ensuring consistent timezone handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for timestamp-timezone handling in the codebase
# Look for timestamp conversion patterns
rg -A 3 "new Date\(|Date\.parse\(|toISOString\(|getTime\(" api/src/pdc/services/apdf/
# Look for timezone conversion utilities
rg -A 3 "timezone|tz\." api/src/pdc/services/apdf/
Length of output: 24263
Script:
#!/bin/bash
# Check for timezone-related utilities and their usage
ast-grep --pattern 'fromZonedTime($_)'
# Check for timestamp type definition
rg -A 3 "type Timestamp" api/src/pdc/services/apdf/
Length of output: 5089
Script:
#!/bin/bash
# Look for the Timestamp type definition in the entire codebase
rg -A 3 "export.*Timestamp" api/src/
# Check for date/time validation and conversion utilities
ast-grep --pattern 'class $_Timestamp {
$$$
}'
# Look for timezone-related imports
rg "import.*timezone" api/src/pdc/services/apdf/
Length of output: 2002
api/src/pdc/services/cee/dto/SimulateApplication.ts (2)
11-11: LGTM! Type export follows best practices
The type inference and export are correctly implemented using Superstruct's Infer utility, providing proper type safety for the validation structure.
4-10: Verify validation constraints for imported types
The structure looks well-organized, but we should verify the validation rules defined in the imported types to ensure they meet the business requirements for application simulation.
✅ Verification successful
Based on the shell script results, I can now generate the final response as the validation rules and constraints are clearly visible in the codebase.
Validation constraints are well-defined and appropriate
The validation types used in SimulateApplication have clear and strict constraints:
DrivingLicenseId: Validates against three patterns - 12 digits, 1-15 alphanumeric chars followed by 4 digits, or 1-15 alphanumeric chars, or '99-' prefixIdentityKey: Must be a 64-character hexadecimal stringLastNameTrunc: Must be exactly 3 uppercase letters or spacesPhone: Must start with '+' followed by numbers onlyJourneyType: Must be either 'short' or 'long'
The structure follows the business requirements with appropriate optionality for driving_license and identity_key fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check validation rules in imported types
# Check shared validation types
echo "Checking validation types..."
ast-grep --pattern 'export const $type = $definition' ./api/src/pdc/providers/superstruct/shared/index.ts
ast-grep --pattern 'export const $type = $definition' ./api/src/pdc/services/cee/dto/shared.ts
# Look for any existing usage patterns
echo "Checking existing usage patterns..."
rg -A 5 "DrivingLicenseId|IdentityKey|LastNameTrunc|Phone|JourneyType" --type ts
Length of output: 51037
api/src/pdc/providers/superstruct/shared/index.ts (2)
1-2: LGTM! Well-structured imports
The imports are well-organized, using absolute paths and importing specific utilities needed for validation.
1-19: Verify consistent validation usage across the codebase
Let's ensure these validation patterns are consistently used and don't conflict with existing validators.
✅ Verification successful
Validation patterns are consistent and properly centralized
The verification shows that the validation patterns in the shared index file are properly used across the codebase:
- The
IdentityKeypattern/^[a-f0-9]{64}$/is consistently used in both superstruct and acquisition service - The
Phonepattern/^\+[0-9]*$/is used consistently in both backend and frontend validations - The UUID pattern is unique to this shared file and not duplicated elsewhere
- Other validation patterns found in the codebase are for different purposes and don't conflict with these shared validators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing validation patterns
echo "Searching for existing validation patterns..."
rg -t typescript "pattern\(.*string\(\).*," --glob '!api/src/pdc/providers/superstruct/shared/index.ts'
# Search for potential UUID validations
echo "Searching for UUID validations..."
rg -t typescript "UUID|uuid" -B 2 -A 2
# Search for phone number validations
echo "Searching for phone validations..."
rg -t typescript "phone.*validation|validatePhone|PhoneNumber" -B 2 -A 2
Length of output: 583
Script:
#!/bin/bash
# Search for existing validation patterns without file type restriction
echo "Searching for existing validation patterns..."
rg "pattern\(.*string\(\).*," --glob '!api/src/pdc/providers/superstruct/shared/index.ts'
# Search for UUID pattern validations
echo "Searching for UUID validations..."
rg "[Uu]uid.*pattern|pattern.*[Uu]uid" -B 2 -A 2
# Search for phone number pattern validations
echo "Searching for phone pattern validations..."
rg "phone.*pattern|pattern.*phone" -i -B 2 -A 2
# Search for identity key validations
echo "Searching for identity key validations..."
rg "identitykey|identity.*key.*pattern" -i -B 2 -A 2
# Search for timestamp validations
echo "Searching for timestamp validations..."
rg "timestamp.*pattern|pattern.*timestamp" -i -B 2 -A 2
Length of output: 16683
api/src/pdc/services/cee/actions/DeleteCeeAction.ts (3)
4-4: LGTM! Good use of DTO for type safety
The introduction of a dedicated DTO for delete operations improves type safety and validation.
9-11: Verify API compatibility with consumers
The method name change from the previous version might affect API consumers.
Let's check for existing usage:
✅ Verification successful
No breaking changes detected in API method name
The method name deleteCeeApplication is already in use and consistent across the codebase:
- The HTTP route in
HttpTransport.tsusescee:deleteCeeApplication - The handler in
DeleteCeeAction.tsis correctly decorated with this method name - The internal repository method
deleteCeeByUuidremains unchanged
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential API calls to the old method name
rg -l "deleteCee[^A]" --type ts
# Search for potential service client configurations
rg -l "service:\s*['\"]cee['\"]" --type ts
Length of output: 774
Script:
#!/bin/bash
# Let's check the actual implementation and usage of the delete method
ast-grep --pattern 'deleteCee' -l
rg 'deleteCee' -B2 -A2 --type ts
Length of output: 4183
21-23: Consider error handling implications of void return
The change from Promise<ResultInterface> to Promise<void> might affect error handling in API consumers. Consider:
- How are successful deletions communicated to the client?
- Are consumers expecting a confirmation response?
Let's check the repository implementation:
✅ Verification successful
Return type change is consistent with error handling patterns
The change from Promise<ResultInterface> to Promise<void> aligns with the codebase's error handling patterns:
- The repository layer (
CeeRepositoryProvider) already usesPromise<void>for delete operations - Error cases are handled through exceptions (e.g.,
NotFoundException,ConflictException) - Successful deletion is implicitly communicated by the absence of an exception
- This pattern is consistent with other delete operations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check repository implementation for error handling
ast-grep --pattern 'deleteCeeByUuid($_,$_) {
$$$
}'
# Look for similar delete operations in other services
rg "delete.*ByUuid.*Promise<void>" --type ts
Length of output: 386
Script:
#!/bin/bash
# Check for API routes and controllers using this action
rg "DeleteCeeAction" -A 5 --type ts
# Check for similar delete patterns in other actions
ast-grep --pattern 'class $_ implements ActionInterface {
$$$
async execute($_): Promise<void> {
$$$
}
}'
# Look for error handling patterns in the codebase
rg "throw new \w+Exception" --type ts
Length of output: 20720
api/src/pdc/services/cee/dto/RegisterApplication.ts (2)
1-24: LGTM! Well-structured type definition.
The type definition is well-organized with:
- Clear separation between common and journey-specific fields
- Proper use of literal types for journey differentiation
- Type-safe exports using
Infer
9-10: Verify PII data handling compliance.
The schema includes truncated PII data (last_name_trunc, driving_license). Ensure that:
- The truncation methods are consistent with data protection requirements
- The data retention policies are properly documented
- The purpose of storing this data is justified and documented
Also applies to: 19-20
api/src/pdc/services/application/actions/RevokeApplicationAction.ts (2)
1-6: LGTM: Clean import organization
The imports are well-structured and properly aligned with the new validation approach using DTOs.
Line range hint 28-35: Verify the impact of removing the result interface
The change from Promise<ResultInterface> to Promise<void> suggests this is now a fire-and-forget operation. Ensure this aligns with the service's error handling and client notification requirements.
✅ Verification successful
The previous pattern didn't yield results, so I'm trying a different approach:
- Looking for handle methods with return types in action classes
- Finding where RevokeApplicationAction is used to understand the impact
- Checking ResultInterface usage in the codebase
This will help us verify if removing ResultInterface is safe and aligns with other patterns in the codebase.
Based on the search results, I can now provide a definitive response about the return type change:
Return type change from Promise<ResultInterface> to Promise<void> is safe and aligns with the codebase patterns
The codebase shows that both return types (void and ResultInterface) are used across different action handlers, with several examples of void returns in similar command-pattern implementations:
IndexAllGeoAction.handle(): Promise<void>StatsAction.handle(): Promise<void>NotifyUserAction.handle(): Promise<void>
The change aligns with the command pattern where operations that only perform state changes (like revoking) don't need to return data. The owner_service hardcoding is also consistent with the service's role-based architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar patterns in other actions
# Look for other action handlers returning void
ast-grep --pattern 'class $_ extends AbstractAction {
$$$
async handle($_): Promise<void> {
$$$
}
}'
Length of output: 112
Script:
#!/bin/bash
# Search for handle methods with return types in action classes
rg -U "class.*Action.*\{[\s\S]*?handle.*Promise<.*>.*\{" --type ts
# Search for usages of RevokeApplicationAction to understand impact
rg "RevokeApplicationAction" --type ts -A 3
# Search for ResultInterface usage
rg "ResultInterface" --type ts
Length of output: 132926
api/src/pdc/services/application/ApplicationServiceProvider.ts (2)
Line range hint 1-28: LGTM! Clean implementation of the service provider.
The service provider implementation follows best practices with:
- Clear separation of concerns
- Proper middleware registration
- Well-organized action handlers
- Clean inheritance structure
5-5: Verify the impact of the new validator middleware.
The switch to the superstruct-based ValidatorMiddleware represents a significant change in validation strategy. Let's verify that all action handlers are compatible with this change.
Also applies to: 16-18
✅ Verification successful
The new ValidatorMiddleware implementation is properly integrated and compatible with all action handlers
All action handlers are using superstruct-based validation schemas through their respective DTOs:
- CreateApplication uses superstruct's object, array, and custom types
- FindApplication uses superstruct's object, union, and custom types
- ListApplication uses superstruct's object and custom types
- RevokeApplication uses superstruct's object and custom types
The middleware is correctly registered in ApplicationServiceProvider and each action handler properly references its corresponding DTO for validation using the ["validate", DTO] middleware syntax.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all action handlers have updated their validation schemas for superstruct
# Look for validation schema definitions in action handlers
echo "Checking validation schemas in action handlers..."
ast-grep --pattern 'schema = {
$$$
}' api/src/pdc/services/application/actions/
# Check for any remaining old validator imports or usage
echo "Checking for old validator patterns..."
rg -l "import.*validator" api/src/pdc/services/application/actions/
Length of output: 349
Script:
#!/bin/bash
# Let's try a different approach to verify the validation middleware usage
# First, let's see what action handlers exist
echo "Finding all action handler files..."
fd -e ts . api/src/pdc/services/application/actions/
# Check for any validation-related imports or usage patterns
echo "Checking validation patterns..."
rg -A 5 "schema.*=|validate.*=|validator" api/src/pdc/services/application/actions/
# Check for the new ValidatorMiddleware usage
echo "Checking ValidatorMiddleware usage..."
rg -A 5 "ValidatorMiddleware" api/src/pdc/services/application/
Length of output: 2044
Script:
#!/bin/bash
# Let's examine the action handlers more closely to understand their validation approach
# Check the content of each action handler
echo "Content of CreateApplicationAction.ts:"
cat api/src/pdc/services/application/actions/CreateApplicationAction.ts
echo -e "\nContent of FindApplicationAction.ts:"
cat api/src/pdc/services/application/actions/FindApplicationAction.ts
echo -e "\nContent of ListApplicationAction.ts:"
cat api/src/pdc/services/application/actions/ListApplicationAction.ts
echo -e "\nContent of RevokeApplicationAction.ts:"
cat api/src/pdc/services/application/actions/RevokeApplicationAction.ts
# Also check the ValidatorMiddleware implementation
echo -e "\nContent of ValidatorMiddleware:"
cat api/src/pdc/providers/superstruct/ValidatorMiddleware.ts
Length of output: 6610
Script:
#!/bin/bash
# Let's check the DTO files that contain the actual validation schemas
echo "Content of CreateApplication DTO:"
cat api/src/pdc/services/application/dto/CreateApplication.ts
echo -e "\nContent of FindApplication DTO:"
cat api/src/pdc/services/application/dto/FindApplication.ts
echo -e "\nContent of ListApplication DTO:"
cat api/src/pdc/services/application/dto/ListApplication.ts
echo -e "\nContent of RevokeApplication DTO:"
cat api/src/pdc/services/application/dto/RevokeApplication.ts
Length of output: 2067
api/src/pdc/services/apdf/APDFServiceProvider.ts (2)
30-30: LGTM: Clean class implementation
The empty class extending AbstractServiceProvider is appropriate as all configuration is handled through the decorator.
10-10: Verify the new validation approach implementation
The migration to @/pdc/providers/superstruct/ValidatorMiddleware.ts aligns with the broader refactoring effort to streamline validation. Let's verify that all validation rules have been properly migrated.
✅ Verification successful
Migration to superstruct validator is correctly implemented
The verification shows that:
- The new
ValidatorMiddlewarefrom superstruct is properly implemented with schema validation and error handling - APDFServiceProvider correctly imports and configures the new middleware
- The migration pattern is consistent across multiple services, with 15 services already using the new
ValidatorMiddleware - The old validator references found are primarily in core framework files and types, which is expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the validation rules migration and usage
# Check for any remaining old validator references
echo "Checking for old validator references..."
rg -l "ValidatorExtension|NewableType"
# Check the new validator implementation
echo "Checking new validator implementation..."
ast-grep --pattern 'class ValidatorMiddleware {
$$$
}'
Length of output: 2259
Script:
#!/bin/bash
# Let's check the actual implementation and usage of the new ValidatorMiddleware
# Check the implementation of ValidatorMiddleware
echo "Checking ValidatorMiddleware implementation..."
rg -A 10 "class ValidatorMiddleware"
# Check how APDFServiceProvider uses the ValidatorMiddleware
echo "Checking APDFServiceProvider usage..."
rg -A 10 "ValidatorMiddleware" api/src/pdc/services/apdf/APDFServiceProvider.ts
# Check if other services have already migrated to the new middleware
echo "Checking adoption of new ValidatorMiddleware..."
rg -l "ValidatorMiddleware" api/src/pdc/services/
Length of output: 3710
api/src/pdc/services/application/actions/ListApplicationAction.ts (3)
1-7: LGTM! Clean import organization
The imports are well-structured using absolute paths and clearly separate core functionality from domain-specific imports.
9-21: Verify the permission scope string
The middleware chain looks secure, but let's verify that the permission scope operator.application.list is consistently defined across the system.
✅ Verification successful
Permission scope operator.application.list is consistently defined and used
The permission scope is correctly used across the codebase:
- In
ListApplicationAction.tsas the required permission scope for the list operation - In
DeleteOperatorAction.tsas part of operator permissions setup - In
ApplicationService.integration.spec.tsfor testing with the same permission
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of this permission scope to ensure consistency
rg "operator\.application\.list" --type ts
Length of output: 397
Script:
#!/bin/bash
# Let's check the context of these usages to understand if they are consistently used for the same purpose
rg "operator\.application\.list" --type ts -B 2 -A 2
Length of output: 1561
Line range hint 29-36: Consider making owner_service configurable
The hardcoded owner_service: "operator" might limit reusability. Consider making this configurable through the DTO or service configuration.
api/src/pdc/services/application/actions/CreateApplicationAction.ts (2)
1-7: LGTM: Import changes improve type safety
The transition from generic interfaces to specific types (CreateApplication DTO) and concrete implementations (ApplicationPgRepositoryProvider) enhances type safety and maintainability.
9-21: LGTM: Handler configuration with proper authorization chain
The middleware chain implements a robust security model:
- Context data propagation
- Permission scope verification
- Input validation using strongly-typed DTO
Let's verify the permission scope consistency across the codebase:
✅ Verification successful
Permission scopes are consistently defined and used across the application service
The verification shows that:
- The
operator.application.createpermission scope is consistently used in both the action handler and integration tests - It's part of a complete set of application-related permission scopes including:
operator.application.createoperator.application.findoperator.application.listoperator.application.revoke
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the permission scope usage
# Expected: The permission scope should be consistently used across the codebase
# Check for permission scope usage
rg -A 2 "operator\.application\.create"
# Check for other application-related permission scopes
rg "operator\.application\."
Length of output: 3082
api/src/pdc/services/application/actions/FindApplicationAction.ts (3)
1-7: LGTM: Clean and well-organized imports
The imports are properly organized and align well with the new architecture, using consistent alias paths.
Line range hint 23-37: Consider making owner_service configurable
The hardcoded owner_service: "operator" in the repository call might limit the reusability of this action. Consider making this configurable if there's a possibility of other owner services in the future.
9-21: Verify the permission scope chain
The middleware chain implements proper security with permission checks, but let's verify the complete scope chain and its usage across the codebase.
✅ Verification successful
Permission scope chain is correctly implemented and consistently used
The verification results show that:
- The permission scopes
proxy.application.findandoperator.application.findare consistently used across the codebase - The scope chain is properly implemented in
serverTokenMiddleware.tsfor proxy permissions - Other application actions (Create, List, Revoke) follow similar security patterns with
hasPermissionByScopeMiddleware - Integration tests validate the operator permissions correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the permission scope chain and its consistent usage
# Check for other usages of these permission scopes
echo "Checking permission scope usage patterns..."
rg -A 2 "proxy\.application\.find|operator\.application\.find"
# Verify the middleware implementation
echo "Checking hasPermissionByScopeMiddleware implementation..."
ast-grep --pattern 'hasPermissionByScopeMiddleware($scope, $perms)'
# Look for similar patterns in other application-related actions
echo "Checking similar patterns in application actions..."
fd --type f ".*Action\.ts$" | grep -i "application" | xargs rg "hasPermissionByScopeMiddleware"
Length of output: 2998
api/src/pdc/services/cee/actions/SimulateCeeAction.ts (3)
7-7: LGTM: Import of new DTO is well-structured
The introduction of a specific DTO (SimulateApplication) aligns with the broader validation refactor and follows TypeScript best practices.
16-18: Verify service consumer compatibility with updated handler configuration
The handler configuration looks good, but we should verify that all service consumers are updated to use the new method name "simulateCeeApplication".
✅ Verification successful
Service consumer compatibility is verified
The changes are consistent across the codebase:
- The schema file defines the alias as
cee.simulateCeeApplication - The contract file uses the same method name in
handlerConfig - The HTTP transport routes correctly map to
cee:simulateCeeApplication - No references to old method names were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to the CEE simulation endpoint
rg -l "simulateCeeApplication|cee\.simulate" --type ts
# Search for potential service calls that might need updating
ast-grep --pattern 'kernel.call($service, $method, $$$)' --type ts
Length of output: 478
Script:
#!/bin/bash
# Let's examine the content of these files to verify the service consumer compatibility
cat shared/cee/simulateApplication.schema.ts shared/cee/simulateApplication.contract.ts api/src/pdc/proxy/HttpTransport.ts
# Let's also search for any potential old method name references
rg "cee\.simulate" -A 2 -B 2
Length of output: 32586
29-31: 🛠️ Refactor suggestion
Document error cases and verify return type change impact
The change from Promise<ResultInterface> to Promise<void> is a breaking change. While the error handling through exceptions is maintained, consider:
- Adding JSDoc comments to document the possible exceptions (
ServiceDisabledExceptionandConflictException) - Verifying that all consumers handle the void return type correctly
Consider adding documentation:
+/**
+ * Simulates a CEE application and validates against cooldown constraints
+ * @throws {ServiceDisabledException} When CEE import is disabled
+ * @throws {ConflictException} When a conflicting application exists
+ */
public async handle(
params: SimulateApplication,
context: ContextType,
): Promise<void>✅ Verification successful
Based on the search results, I can now generate the final response:
Return type change is safe to proceed
The change from Promise<ResultInterface> to Promise<void> appears safe as there are no consumers depending on the return value. The search results show:
- The method is only referenced in schema/contract definitions and transport configuration
- No instances found of code awaiting and using the return value
- The API endpoint is configured with HTTP 200 success code without expecting response data
Adding the suggested JSDoc documentation for exceptions would still be valuable for maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls that might expect a return value
rg -A 5 "simulateCeeApplication|cee\.simulate.*\=" --type ts
# Look for potential await usage that expects a return value
ast-grep --pattern 'const $result = await $$$simulateCeeApplication$$$' --type ts
Length of output: 471
Script:
#!/bin/bash
# Search for calls that might expect a return value
rg "simulateCeeApplication|cee\.simulate" -A 5 --type typescript
# Look for potential await usage that expects a return value
ast-grep --pattern 'const $_ = await $$$simulateCeeApplication$$$'
Length of output: 77094
Script:
#!/bin/bash
# Search for calls to simulateCeeApplication in TypeScript files
rg "simulateCeeApplication" --type ts -A 5
# Look for potential await usage that expects a return value
ast-grep --pattern 'await $$$simulateCeeApplication$$$'
Length of output: 1804
api/src/pdc/services/acquisition/dto/shared.ts (5)
1-16: LGTM: Imports are well-organized
The imports are properly structured, importing only the necessary validation primitives and re-exporting shared types.
26-26: Phone validation needs strengthening
43-43: Fix typo in Identity field name
51-62: LGTM: Comprehensive status enumeration
The journey statuses are well-defined with clear error states and lifecycle flags.
70-71: LGTM: Reasonable pagination constraints
The pagination limits are well-defined with sensible constraints.
api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts (2)
73-73: LGTM!
The interface extension is well-defined and consistent with the Date type usage pattern.
10-10:
Verify the impact of Timestamp type change
Changing Timestamp from string to Date is a breaking change that could affect serialization/deserialization across the application. There's also an inconsistency in its usage (e.g., CeeApplicationResultInterface.datetime remains as string).
api/src/pdc/services/cee/actions/FindCeeAction.ts (2)
6-6: LGTM: Import changes align with type updates
The new imports correctly support the transition to more specific types for the action.
Also applies to: 9-9
13-15: Verify the impact of the method name change
The handler method name has been updated. Please ensure this change is reflected in all API consumers to prevent breaking changes.
api/src/pdc/services/apdf/actions/ListAction.ts (2)
31-40: LGTM! Permission configuration looks comprehensive.
The handler configuration properly implements:
- Service and method specification
- Granular permissions for territory, operator, and registry access
- Validation using the new
ListApdfDTO
Let's verify the permission configuration is consistent across the codebase:
✅ Verification successful
Let me gather more information to verify the permission configuration consistency across APDF-related actions.
Let me check one more thing to verify if there are any other APDF-related actions and their permission patterns that might be in different files.
Permission configuration is correctly implemented and consistent
The APDF service has two actions with appropriate permission configurations:
ListAction: Uses granular permissions (territory, operator, registry) viacopyGroupIdAndApplyGroupPermissionMiddlewaresExportAction: Uses internal-only access restriction viainternalOnlyMiddlewares
Each action follows the correct permission pattern for its use case - public-facing list operation with proper access controls, and restricted export operation for internal use only.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar permission patterns in other APDF-related actions
rg -A 5 "territory.apdf" api/src/pdc/services/apdf/
Length of output: 496
Script:
#!/bin/bash
# Search for all APDF-related actions and their permission patterns
rg -A 10 "service: \"apdf\"" api/src/pdc/services/apdf/actions/
Length of output: 2654
Script:
#!/bin/bash
# Find all action files in the APDF service directory
fd -e ts . api/src/pdc/services/apdf/actions/ --exec grep -l "service: \"apdf\"" {} \;
# Search for any group permission middleware usage in these files
rg -A 5 "copyGroupIdAndApplyGroupPermissionMiddlewares|internalOnlyMiddlewares" api/src/pdc/services/apdf/actions/
Length of output: 2697
52-54: Verify the ListApdf DTO implementation.
The method signature change looks good, using the new ListApdf DTO.
Let's verify the DTO implementation:
✅ Verification successful
ListApdf DTO implementation is properly defined and used
The DTO is correctly implemented in api/src/pdc/services/apdf/dto/ListApdf.ts using Zod schema with the following required fields:
campaign_id: Serialoperator_id: optional Serialterritory_id: optional Serial
The type is properly exported and imported in the ListAction, maintaining type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the ListApdf DTO implementation
ast-grep --pattern 'export class ListApdf {
$$$
}'
Length of output: 54
Script:
#!/bin/bash
# Let's try searching for ListApdf type/interface/class with different approaches
echo "Searching for ListApdf type definition..."
rg "ListApdf" -A 5
echo -e "\nSearching for any file containing ListApdf in its name..."
fd "ListApdf"
echo -e "\nSearching for type/interface definition..."
ast-grep --pattern 'type ListApdf = {
$$$
}'
ast-grep --pattern 'interface ListApdf {
$$$
}'
Length of output: 2372
api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts (1)
4-11: Consider maintaining interface implementation for type safety
The removal of ApplicationRepositoryProviderInterface implementation might reduce type safety. While using DTOs improves parameter typing, implementing the interface ensures the class maintains the expected contract.
Let's verify if this change affects other components:
api/src/pdc/providers/carpool/repositories/CarpoolStatusRepository.ts (1)
71-71: Verify the impact of type change across the codebase.
The change from OperatorJourneyId to string type appears to be part of a broader restructuring of journey management. While this change makes the interface more flexible, we should ensure consistency across the codebase.
Let's verify the usage and type consistency:
✅ Verification successful
Type change is consistent with the codebase design
The change from OperatorJourneyId to string is actually aligned with the codebase's type definitions. The search results show that OperatorJourneyId is defined as a type alias for string in multiple locations:
shared/cee/common/CeeApplicationInterface.ts:export type OperatorJourneyId = string;api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts:export type OperatorJourneyId = string;api/src/pdc/services/acquisition/dto/shared.ts:export const OperatorJourneyId = ExternalId;
Since OperatorJourneyId is just a type alias for string, using string directly maintains the same type safety while reducing an unnecessary layer of abstraction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of OperatorJourneyId type and
# verify consistent typing across journey-related code
# Check for any remaining OperatorJourneyId type references
echo "Checking for OperatorJourneyId type usage:"
rg "OperatorJourneyId" -t ts
# Check for other methods using operator_journey_id to verify type consistency
echo "Checking operator_journey_id parameter types in methods:"
ast-grep --pattern 'operator_journey_id: $_'
Length of output: 4035
api/src/pdc/services/apdf/providers/excel/BuildExcel.ts (1)
Line range hint 1-124: PR description needs more context about the validation changes.
While the code changes in this file are minimal, the PR title suggests validator changes. It would be helpful to have more context in the PR description about:
- The motivation behind moving interfaces out of the contracts directory
- How this relates to the broader validation changes
- Any breaking changes that consumers of these interfaces should be aware of
api/src/pdc/services/apdf/actions/ExportAction.ts (3)
8-8: LGTM! Type definitions are clear and well-aligned
The new ExportApdf import and explicit ResultInterface type definition improve type safety and code clarity. The string[] return type accurately represents the array of file paths returned by the handle method.
Also applies to: 16-16
19-22: Verify the ExportApdf validation schema
The handler now uses ExportApdf for request validation. Let's ensure the validation schema is properly defined.
✅ Verification successful
ExportApdf validation schema is properly defined
The validation schema is well-structured with:
- Required
campaign_idarray field of Serial type - Optional
operator_idarray field - Optional date range with start/end timestamps
- Optional timezone formatting configuration
The schema is correctly imported and used in the ExportAction handler, maintaining proper type safety and validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the ExportApdf DTO definition
# Expected: Should find the validation schema with required campaign_id field
ast-grep --pattern 'export class ExportApdf {
$$$
}'
# Also check for any other uses of this DTO
rg -l "ExportApdf"
Length of output: 169
Script:
#!/bin/bash
# Let's check the actual content of the ExportApdf DTO
cat api/src/pdc/services/apdf/dto/ExportApdf.ts
# Also check the imports in ExportAction to ensure proper usage
cat api/src/pdc/services/apdf/actions/ExportAction.ts
Length of output: 6057
36-37: Verify backward compatibility and consider error handling improvements
The parameter type change from ParamsInterface to ExportApdf looks good, but let's ensure all callers have been updated. Additionally, consider adding more structured error handling:
- Consider grouping related error messages
- Add specific error types for different failure scenarios
Consider adding structured error handling:
+class ApdfExportError extends Error {
+ constructor(
+ message: string,
+ public readonly campaignId: string,
+ public readonly operatorId?: string
+ ) {
+ super(message);
+ }
+}
public async handle(
params: ExportApdf,
context: ContextType,
): Promise<ResultInterface> {
try {
// ... existing code ...
} catch (error) {
- logger.error(`[apdf:export] Export failed: ${error.message}`);
+ if (error instanceof ApdfExportError) {
+ logger.error(
+ `[apdf:export] Export failed for campaign ${error.campaignId}` +
+ `${error.operatorId ? `, operator ${error.operatorId}` : ''}: ${error.message}`
+ );
+ }
throw error;
}
}api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts (3)
Line range hint 23-65: Well-structured test setup with comprehensive fixtures
The test setup is thorough, including:
- Database and kernel context initialization
- Comprehensive test payloads for both journey types
- Proper crypto signing setup
- Clean teardown in afterAll
Line range hint 1-289: Overall test coverage is comprehensive and well-structured
The test suite provides thorough coverage of:
- Input validation
- Authorization
- Success scenarios
- Error handling
- Database state verification
The changes align well with the codebase restructuring while maintaining test integrity.
16-21: Verify import path changes across the codebase
The restructuring from contract-based to interface-based organization needs verification to ensure consistent usage across the codebase.
✅ Verification successful
Import changes are consistent with codebase restructuring
The import changes in the integration test file are correct and align with the codebase structure. All related files are using the proper import paths:
RegisterCeeAction.integration.spec.tscorrectly imports fromcontracts/registerApplication.contract.tsFindCeeAction.tscorrectly imports frominterfaces/CeeApplicationInterface.ts- No files are using the old
contracts/common/CeeApplicationInterfacepath
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old contract path and verify consistent interface usage
# Check for any remaining references to the old contract path
rg -l "contracts/common/CeeApplicationInterface"
# Check for consistent usage of the new interface path
rg -l "interfaces/CeeApplicationInterface"
Length of output: 266
Script:
#!/bin/bash
# Let's check the actual content of these files to verify the import statements
echo "=== RegisterCeeAction.ts ==="
rg "import.*CeeApplicationInterface" api/src/pdc/services/cee/actions/RegisterCeeAction.ts -A 1
echo -e "\n=== RegisterCeeAction.integration.spec.ts ==="
rg "import.*CeeApplicationInterface" api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts -A 1
echo -e "\n=== FindCeeAction.ts ==="
rg "import.*CeeApplicationInterface" api/src/pdc/services/cee/actions/FindCeeAction.ts -A 1
Length of output: 716
Script:
#!/bin/bash
# Let's check the complete imports section of these files to get a better picture
echo "=== RegisterCeeAction.ts imports ==="
rg "^import" api/src/pdc/services/cee/actions/RegisterCeeAction.ts
echo -e "\n=== RegisterCeeAction.integration.spec.ts imports ==="
rg "^import" api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts
echo -e "\n=== FindCeeAction.ts imports ==="
rg "^import" api/src/pdc/services/cee/actions/FindCeeAction.ts
Length of output: 2642
api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts (1)
4-4: Verify consistent interface import paths across the codebase
The interface has been moved from contracts/interfaces to interfaces. Let's ensure this structural change is consistent across all files.
api/src/pdc/providers/superstruct/shared/tz.ts (1)
615-615: LGTM! Type export is correctly implemented.
The type definition using Infer ensures type safety and proper inference from the enum.
api/src/pdc/proxy/HttpTransport.ts (3)
44-44: LGTM!
The import path change for TokenPayloadInterface is correctly updated.
Line range hint 340-380: LGTM: Well-structured CEE route implementations.
The CEE routes demonstrate good practices:
- RESTful endpoint design
- Proper HTTP method selection
- Explicit success status codes
- Consistent error handling through the transport layer
Line range hint 340-380: Review rate limiting configuration for CEE routes.
While the route changes look good, the rate limit of 20,000 requests per minute (333 req/sec) seems unusually high for CEE-related operations. This could potentially lead to:
- Excessive server load during peak times
- Increased risk of DoS attacks
- Database connection pool exhaustion
Consider adjusting the rate limits to more conservative values based on expected traffic patterns.
Let's verify the rate limits across other similar endpoints:
api/src/pdc/services/cee/actions/RegisterCeeAction.ts (6)
7-7: Import Statement Confirmation
The addition of UnexpectedException in the import statements appears appropriate for handling unexpected cases in the code.
38-40: Verify Middleware Configuration for Validation
The validate middleware is now applied with RegisterApplication. Confirm that RegisterApplication includes all necessary validation rules to ensure incoming data conforms to expected formats and types.
Line range hint 199-199: Validate Date Range in isBetweenOrFail
When using isBetweenOrFail, ensure that the date range checks are accurate and that the start_date and end_date are correctly configured in validJourneyConstraint. Incorrect date validations could lead to valid journeys being rejected or invalid ones being accepted.
15-15: Verify the Import Path for RegisterApplication
Ensure that the import path for RegisterApplication is correct and that the file exists at @/pdc/services/cee/dto/RegisterApplication.ts to prevent module resolution errors.
Run the following script to confirm the existence of the file:
✅ Verification successful
The previous command didn't work because the @ prefix is likely a path alias. Let's search for the actual file and its imports to verify the correct path and usage.
Import path is correct but can be simplified
The file exists at the correct location api/src/pdc/services/cee/dto/RegisterApplication.ts. The @ prefix in the import is an alias that likely maps to api/src. The import works, but the .ts extension in the import statement is unnecessary and can be removed according to TypeScript conventions.
api/src/pdc/services/cee/actions/RegisterCeeAction.ts: Consider updating the import toimport { RegisterApplication } from "@/pdc/services/cee/dto/RegisterApplication";
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that RegisterApplication.ts exists at the specified path.
fd 'RegisterApplication.ts' | grep '@/pdc/services/cee/dto/RegisterApplication.ts'
Length of output: 80
Script:
#!/bin/bash
# Let's search for RegisterApplication.ts without the @ prefix since it might be an alias
fd "RegisterApplication.ts"
# Also search for any imports of RegisterApplication to understand its usage
rg "RegisterApplication" -t ts
Length of output: 658
105-105: Ensure Safe Access to application_id in Error Data
Accessing e.rpcError?.data?.uuid assumes that, in the case of a ConflictException, these properties exist. Confirm that ConflictException instances will always have rpcError.data.uuid populated, or add additional checks to handle cases where these properties may be undefined to prevent potential runtime errors.
Run the following script to check the structure of ConflictException:
✅ Verification successful
Let me check how the ConflictException is being used in the codebase, particularly in the context of RegisterCeeAction.
Optional uuid in ConflictException Data is Correctly Handled
The current implementation is safe. From the codebase analysis:
- The
ConflictExceptionconstructor accepts optional data that's assigned torpcError.data - In
RegisterCeeAction.ts,ConflictExceptionis thrown with explicituuidin some cases ({ uuid: old.uuid, ... }), while in others without it - The optional chaining (
?.) ine.rpcError?.data?.uuidcorrectly handles both cases where uuid may or may not be present
The current implementation appropriately uses optional chaining to safely access the potentially undefined uuid property, making it a robust solution for handling both cases where the uuid is present or absent in the error data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `ConflictException` instances contain `rpcError.data.uuid`.
rg 'class ConflictException' -A 15
Length of output: 906
Script:
#!/bin/bash
# Search for instances where ConflictException is thrown with uuid in data
rg "throw new ConflictException.*uuid" -B 2 -A 2
# Also search for any ConflictException usage in RegisterCeeAction
rg "ConflictException" api/src/pdc/services/cee/actions/RegisterCeeAction.ts -B 2 -A 2
Length of output: 1824
63-65: Ensure Compatibility with Updated Parameter and Return Types
The handle method now accepts RegisterApplication as parameters and returns Promise<CeeApplicationResultInterface>. Verify that all calls to this method have been updated accordingly and that the new DTO aligns with the expected data structure.
Run the following script to identify calls to the handle method:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
Parameter Type Changes Are Properly Handled
The RegisterCeeAction is only used within its own integration tests and is properly registered in the CeeServiceProvider. The integration tests demonstrate that the method is being called with the correct parameter types and the return values are properly validated. The tests cover various scenarios including successful registrations and error cases, showing that the parameter and return types are working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to RegisterCeeAction's handle method.
rg -A 2 'RegisterCeeAction' | rg 'handle\('
Length of output: 43
Script:
#!/bin/bash
# Let's try a different approach to find the usage of RegisterCeeAction and its handle method
# First, let's find files containing RegisterCeeAction
rg -l "RegisterCeeAction"
# Then, let's look for the actual implementation and usage
rg -A 5 "class RegisterCeeAction"
# Also search for potential imports of RegisterCeeAction
rg "import.*RegisterCeeAction"
Length of output: 1020
Script:
#!/bin/bash
# Let's examine the test file and service provider to understand the usage
cat api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts
# Also check the service provider implementation
cat api/src/pdc/services/cee/CeeServiceProvider.ts
Length of output: 10378
Script:
#!/bin/bash
# Let's check the contract file to understand the parameter types and interface
cat api/src/pdc/services/cee/contracts/registerApplication.contract.ts
# Also check the handler configuration
cat api/src/pdc/services/cee/contracts/registerApplication.schema.ts
Length of output: 339
| import { CampaignSearchParamsInterface } from "../../interfaces/APDFRepositoryProviderInterface.ts"; | ||
| import { SliceStatInterface } from "../../interfaces/PolicySliceStatInterface.ts"; |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent import paths need to be updated
The interface import paths are inconsistent across files:
- New path (
interfaces/PolicySliceStatInterface):- BuildExcel.ts
- SlicesWorksheetWriter.ts
- APDFRepositoryProvider.ts
- Old path (
contracts/interfaces/PolicySliceStatInterface):- BuildExcel.unit.spec.ts
- SlicesWorksheetWriter.integration.spec.ts
The test files still use the old import path and need to be updated to maintain consistency with the new structure.
🔗 Analysis chain
Verify consistent interface path updates across the codebase.
The import path change looks correct, but let's ensure this restructuring is consistent across all files using this interface.
Let me check the actual import statements in these files to verify if there's a mix of old and new paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old path and verify consistent usage of the new path
# Check for any remaining references to the old path
echo "Checking for old import path references..."
rg -l "contracts/interfaces/PolicySliceStatInterface"
# Check current usage of the interface
echo "Checking current import patterns..."
rg -l "interfaces/PolicySliceStatInterface"
Length of output: 758
Script:
#!/bin/bash
# Check the actual import statements in the files
echo "=== Checking APDFRepositoryProvider.ts ==="
rg "PolicySliceStatInterface" api/src/pdc/services/apdf/providers/APDFRepositoryProvider.ts -B1 -A1
echo -e "\n=== Checking BuildExcel.ts ==="
rg "PolicySliceStatInterface" api/src/pdc/services/apdf/providers/excel/BuildExcel.ts -B1 -A1
echo -e "\n=== Checking SlicesWorksheetWriter.ts ==="
rg "PolicySliceStatInterface" api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts -B1 -A1
echo -e "\n=== Checking BuildExcel.unit.spec.ts ==="
rg "PolicySliceStatInterface" api/src/pdc/services/apdf/providers/excel/BuildExcel.unit.spec.ts -B1 -A1
echo -e "\n=== Checking SlicesWorksheetWriter.integration.spec.ts ==="
rg "PolicySliceStatInterface" api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.integration.spec.ts -B1 -A1
Length of output: 2174
| @@ -1,5 +1,5 @@ | |||
| import { CarpoolAcquisitionStatusEnum, CarpoolFraudStatusEnum } from "@/pdc/providers/carpool/interfaces/index.ts"; | |||
| import { CeeJourneyTypeEnum, JourneyId } from "../contracts/common/CeeApplicationInterface.ts"; | |||
| import { CeeJourneyTypeEnum, JourneyId } from "./CeeApplicationInterface.ts"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Import path needs to be updated in other files
The import path change in CeeRepositoryProviderInterface.ts is part of a larger refactoring, but there are still files using the old import path ./common/CeeApplicationInterface. The following files need to be updated to use the new import path:
shared/cee/simulateApplication.contract.tsshared/cee/registerApplication.contract.tsshared/cee/importApplicationIdentity.contract.tsshared/cee/importApplication.contract.ts
🔗 Analysis chain
Verify the new import path.
The import path has been changed from a deeper path to a relative one in the same directory. Let's verify this change is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new import file exists and check for any other imports that might need updating
# Check if the new file exists
fd "CeeApplicationInterface.ts" api/src/pdc/services/cee/interfaces/
# Check if there are any other files still using the old import path
rg -l "from ['\"]../contracts/common/CeeApplicationInterface"
Length of output: 197
Script:
#!/bin/bash
# Let's verify the content and imports in both files to ensure they match
# Check the content of CeeApplicationInterface.ts to verify it contains the required types
echo "=== Content of CeeApplicationInterface.ts ==="
cat api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts
# Check if there are any references to these types in other files
echo -e "\n=== Files importing CeeApplicationInterface.ts ==="
rg "from ['\"].*CeeApplicationInterface['\"]" --type ts
Length of output: 3118
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
api/src/pdc/providers/superstruct/shared/index.ts (2)
5-9: Consider making the timestamp validation more flexibleThe current timestamp validation might be too restrictive:
- It only accepts one specific format with milliseconds
- It doesn't validate the actual date values (e.g., would accept invalid dates like 2023-13-45)
Consider this alternative implementation:
export const Timestamp = coerce( date(), - pattern(string(), /^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}\.[0-9]{3}Z$/), + pattern(string(), /^\d{4}-(?:0[1-9]|1[0-2])-(?:0[1-9]|[12]\d|3[01])T(?:[01]\d|2[0-3]):[0-5]\d:[0-5]\d(?:\.\d{1,3})?Z$/), (v) => { + const d = new Date(v); + return isNaN(d.getTime()) ? new Error('Invalid date') : d; }, );
10-10: Consider making Varchar length configurableThe 256-character limit is hardcoded, which might not suit all use cases.
Consider making it configurable:
-export const Varchar = size(string(), 0, 256); +export const createVarchar = (maxLength = 256) => size(string(), 0, maxLength); +export const Varchar = createVarchar();api/src/pdc/services/acquisition/dto/CreateJourney.ts (1)
39-42: Consider making travel pass types configurableThe travel pass name is hardcoded to only accept "navigo". Consider moving this to a configuration if more pass types might be added in the future.
api/src/pdc/services/acquisition/dto/shared.ts (4)
21-24: Consider using a simpler email validation patternThe current email regex is complex and hard to maintain. Consider using a simpler pattern or a dedicated email validation library.
-export const Email = pattern( - Varchar, - /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/, -); +export const Email = pattern(Varchar, /^[^@]+@[^@]+\.[^@]+$/);
20-20: Document the business reason for Amount limitThe maximum amount of 100,000 seems arbitrary. Consider adding a comment explaining the business reason for this limit.
+// Maximum amount set to 100,000 as per business requirement [add reference/reason] export const Amount = size(number(), 0, 100_000);
44-45: Document the purpose of duplicate phone fieldsThe presence of both
phoneandphone_truncfields is unclear. Please add documentation explaining their distinct purposes.
63-63: Document the significance of index limitThe maximum index value of 19 seems arbitrary. Please add a comment explaining this specific limit.
api/src/pdc/services/apdf/providers/CheckCampaign.ts (3)
24-24: Consider making the service identifier configurable.The service identifier "apdf" has been hardcoded, which reduces the flexibility and reusability of this class. Consider injecting this value through the constructor or maintaining it as a configurable parameter.
- channel: { service: "apdf" }, + channel: { service: this.service },And add to the class:
constructor( private kernel: KernelInterfaceResolver, private service: string = 'apdf' ) {}
Line range hint
33-46: Consider using custom error types for better error handling.While the error messages are descriptive, using generic Error instances makes it harder for consumers to handle specific error cases. Consider creating custom error types for different validation scenarios.
Example implementation:
export class CampaignInactiveError extends Error { constructor(campaignId: number) { super(`Campaign ${campaignId} is inactive`); this.name = 'CampaignInactiveError'; } } // Usage in isActive method: throw new CampaignInactiveError(campaign._id);
Line range hint
48-71: Enhance date validation implementation.The current implementation has several areas for improvement:
- The eslint-disable comments are too broad
- Date comparisons could benefit from helper methods
- Error messages format varies between checks
Consider refactoring to:
public isValidDateRange( campaign: GetCampaignResultInterface, start: Date, end: Date, ): void { const { start_date: lower, end_date: upper } = campaign; // Validate existence of dates this.validateDateExists('campaign start_date', lower); this.validateDateExists('campaign end_date', upper); this.validateDateExists('range start', start); this.validateDateExists('range end', end); // Validate date ranges this.validateDateOrder(lower, upper, 'Campaign start', 'campaign end'); this.validateDateOrder(start, end, 'Range start', 'range end'); // Validate range bounds if (end < lower) { throw new Error(`Range end ${end.toISOString()} is before campaign start ${lower.toISOString()}`); } if (start > upper) { throw new Error(`Range start ${start.toISOString()} is after campaign end ${upper.toISOString()}`); } } private validateDateExists(name: string, date: Date | null): void { if (!date) { throw new Error(`Invalid ${name}`); } } private validateDateOrder(first: Date, second: Date, firstName: string, secondName: string): void { if (first >= second) { throw new Error( `${firstName} (${first.toISOString()}) cannot be >= ${secondName} (${second.toISOString()})` ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
api/src/pdc/providers/superstruct/shared/index.ts(1 hunks)api/src/pdc/services/acquisition/dto/CreateJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/shared.ts(1 hunks)api/src/pdc/services/apdf/providers/CheckCampaign.ts(1 hunks)
🔇 Additional comments (14)
api/src/pdc/providers/superstruct/shared/index.ts (3)
1-2: LGTM! Clean imports and exports
The imports are specific and well-scoped, and the use of path alias is a good practice.
4-4: LGTM! Proper PostgreSQL serial constraint
The Serial validator correctly enforces PostgreSQL integer constraints (0 to 2^31-1).
19-19: LGTM! Secure identity key validation
The 64-character hex string validation is appropriate for cryptographic hash representations (32 bytes hex-encoded).
api/src/pdc/services/acquisition/dto/CreateJourney.ts (5)
1-27: LGTM! Well-organized imports
The imports are properly scoped and the use of a local superstruct wrapper enhances maintainability.
45-48: LGTM! Clean and concise schema
The Driver schema is well-structured with appropriate types.
57-62: LGTM! Well-defined constraints
The schema has appropriate constraints for carpooling context with good defaults.
79-79: LGTM! Standard type inference
Proper use of superstruct's Infer type for schema type generation.
29-43: Consider strengthening PII and age verification handling
While the schema includes good practices like optional truncated phone numbers, consider:
- Adding field-level documentation for PII fields to ensure proper handling
- Making
over_18required if age verification is crucial for business logic
Let's verify if age verification is required in the business logic:
✅ Verification successful
Let me analyze the age verification usage in the codebase with additional context:
Let me check the actual implementation of isAdultOrThrow:
Keep over_18 as optional but document its usage in policy rules
The codebase analysis reveals that over_18 is used in policy enforcement where certain territories require adult passengers. The isAdultOrThrow helper is used by multiple policy rules to reject non-adult passengers, but it accepts null values. This indicates that:
- Age verification is policy-specific, not a global requirement
- The current optional schema is correct as it supports both policies that require age verification and those that don't
- The implementation properly handles the optional nature of the field
No changes to the schema are needed, but consider:
- Adding a JSDoc comment to document that this field is required by certain policy rules
- Documenting which operators/territories enforce age restrictions
[documentation_suggested]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for age verification logic in the codebase
rg -g '!*.{json,md}' -i "over_18|age.?verification|minimum.?age"
Length of output: 9063
Script:
#!/bin/bash
# Check policy engine implementation for age verification
ast-grep --pattern 'function isAdultOrThrow($$$) {
$$$
}'
# Check if there are any policy rules that enforce age verification
rg -A 5 "isAdultOrThrow" api/src/pdc/services/policy/engine/policies/
Length of output: 11120
Script:
#!/bin/bash
# Get the implementation of isAdultOrThrow
cat api/src/pdc/services/policy/engine/helpers/isAdultOrThrow.ts
# Check if there are any policy rules that make over_18 required
rg -A 3 "over_18.*required"
Length of output: 586
api/src/pdc/services/acquisition/dto/shared.ts (6)
1-16: LGTM! Clean import structure
The imports are well-organized and the re-exports are properly structured.
29-30: LGTM! Well-defined distance constraint
The distance constraint is reasonable and well-formatted.
31-35: LGTM! Well-structured geo-temporal point
Good composition of previously defined validators into a cohesive structure.
49-60: LGTM! Comprehensive status enumeration
Well-structured enumeration with clear distinction between error and normal states.
68-69: LGTM! Sensible pagination constraints
The pagination limits are reasonable and follow common practices.
46-46: Consider privacy implications of age verification
The over_18 field might store sensitive personal data. Consider adding a comment about data retention policy and GDPR compliance.
| /^[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{12}$/, | ||
| ); | ||
|
|
||
| export const Phone = pattern(Varchar, /^\+[0-9]{6,20}*$/); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen phone number validation
The current regex /^\+[0-9]{6,20}*$/ might be too permissive:
- The
*at the end is redundant - The length range (6-20) might allow invalid numbers
Consider this stricter pattern:
-export const Phone = pattern(Varchar, /^\+[0-9]{6,20}*$/);
+export const Phone = pattern(Varchar, /^\+[1-9][0-9]{7,14}$/);This ensures:
- Numbers start with + followed by non-zero digit
- Total length between 8-15 digits (excluding +)
- No redundant pattern elements
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const Phone = pattern(Varchar, /^\+[0-9]{6,20}*$/); | |
| export const Phone = pattern(Varchar, /^\+[1-9][0-9]{7,14}$/); |
| const Payment = object({ | ||
| amount: Amount, | ||
| type: Varchar, | ||
| index: size(integer(), 0, 19), | ||
| siret: Siret, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider constraining payment types
The type field is currently unconstrained. Consider using an enum to restrict it to valid payment types to prevent invalid values.
- type: Varchar,
+ type: enums(["cash", "card", "transfer"]), // Add valid payment typesCommittable suggestion skipped: line range outside the PR's diff.
| export const CreateJourney = object({ | ||
| operator_id: Serial, | ||
| operator_journey_id: ExternalId, | ||
| operator_trip_id: ExternalId, | ||
| operator_class: OperatorClass, | ||
| start: TimeGeoPoint, | ||
| end: TimeGeoPoint, | ||
| driver: Driver, | ||
| passenger: Passenger, | ||
| distance: Distance, | ||
| licence_plate: optional(Varchar), | ||
| incentives: size(array(Incentive), 0, 20), | ||
| incentive_counterparts: optional(unknown()), | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now generate the final response:
Replace unknown() type with proper CarpoolIncentiveCounterpart structure
The incentive_counterparts field should use the existing CarpoolIncentiveCounterpart type from interfaces/common.ts which has a well-defined structure:
target: IncentiveCounterpartTarget (enum: driver/passenger)siret: Siretamount: Financial
This matches the database schema in operator_incentive_counterparts table and will provide proper type safety and validation.
🔗 Analysis chain
Avoid using unknown type for incentive_counterparts
Using unknown() type reduces type safety. Consider defining a proper structure for incentive_counterparts to ensure proper validation.
Let's check how this field is used:
Let me gather more information about the schema structure by checking the table definition and any related types:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for incentive_counterparts usage
rg -A 5 "incentive_counterparts"
Length of output: 2925
Script:
#!/bin/bash
# Search for any type definitions or interfaces related to incentive counterparts
ast-grep --pattern 'interface $_IncentiveCounterpart {
$$$
}'
# Also check for type definitions
ast-grep --pattern 'type $_IncentiveCounterpart = {
$$$
}'
# Check for any enum or const definitions that might be related
rg -i "incentivecounterpart" -A 5
Length of output: 1513
nmrgt
force-pushed
the
feat/validator
branch
from
d0628e5 to
186ce6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (23)
api/src/pdc/services/apdf/interfaces/StorageRepositoryProviderInterface.ts (1)
Line range hint
7-24: Consider adding JSDoc comments for better documentationThe abstract class methods lack documentation explaining their purpose, parameters, and return values. While the method names are somewhat self-explanatory, proper documentation would improve maintainability.
Example improvement:
export abstract class StorageRepositoryProviderInterfaceResolver { + /** + * Retrieves S3 objects associated with a given campaign + * @param campaign - The serialized policy interface for the campaign + * @returns Promise resolving to a list of S3 objects + */ abstract findByCampaign( campaign: SerializedPolicyInterface, ): Promise<S3ObjectList>;api/src/pdc/services/cee/CeeServiceProvider.ts (1)
Line range hint
1-29: Consider documenting the new validation strategyThe transition to ValidatorMiddleware represents a significant architectural change. Consider:
- Documenting the new validation approach in the project's technical documentation
- Creating migration guides for other teams implementing similar changes
- Adding examples of common validation patterns with the new middleware
Would you like me to help create this documentation?
api/src/pdc/services/apdf/APDFServiceProvider.ts (1)
Line range hint
16-29: Review the middleware configuration orderThe middleware configuration looks good, but consider the order of validation middleware in relation to other middlewares.
Best practices for middleware ordering:
- Authentication/Authorization first
- Validation second
- Business logic middlewares last
The current configuration with
[...defaultMiddlewareBindings, ["validate", ValidatorMiddleware]]appears to follow this pattern, assumingdefaultMiddlewareBindingscontains auth-related middlewares.api/src/pdc/services/apdf/helpers/castExportParams.helper.ts (2)
Line range hint
8-14: Add date order validation.The function should validate that
start_dateis beforeend_dateto prevent invalid date ranges.if (start_date_lc && end_date_lc) { + const start = new Date(start_date_lc); + const end = new Date(end_date_lc); + if (start >= end) { + throw new Error('Start date must be before end date'); + } return { - start_date: new Date(start_date_lc), - end_date: new Date(end_date_lc), + start_date: start, + end_date: end, }; }
Line range hint
9-10: Add input validation for date format.The function should validate the date string format before creating Date objects to prevent invalid date parsing.
+function isValidDateString(date: string): boolean { + const parsed = new Date(date); + return parsed instanceof Date && !isNaN(parsed.getTime()); +} + const start_date_lc = get(params, "query.date.start", null); const end_date_lc = get(params, "query.date.end", null); + +if (start_date_lc && !isValidDateString(start_date_lc)) { + throw new Error('Invalid start date format'); +} +if (end_date_lc && !isValidDateString(end_date_lc)) { + throw new Error('Invalid end date format'); +}api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts (1)
Line range hint
18-39: Architecture change: Centralized validation strategyThe service provider has been refactored to use a centralized validation approach through middleware instead of per-binding validators. This is a good architectural decision as it:
- Reduces code duplication
- Provides a single point of validation logic
- Makes validation rules easier to maintain and update
- Ensures consistent validation behavior across all handlers
The middleware configuration looks correct with ValidatorMiddleware properly included in the chain.
Consider documenting this architectural change in the PR description to help other developers understand the new validation flow.
api/src/pdc/services/acquisition/dto/CreateJourney.ts (1)
29-43: Consider documenting PII handling requirementsThe Identity schema contains sensitive PII data (email, phone, name). Consider adding JSDoc comments to document:
- Data protection/encryption requirements
- Retention policies
- GDPR compliance considerations
Also, the purpose of
phone_truncisn't clear from the name. Consider adding a comment explaining its usage.api/src/pdc/services/acquisition/dto/shared.ts (7)
21-24: Consider using a simpler email validation patternThe current email regex pattern is complex and might be difficult to maintain. Consider using a simpler pattern or a dedicated email validation library.
-export const Email = pattern( - Varchar, - /^(([^<>()\[\]\\.,;:\s@"]+(\.[^<>()\[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))/, -); +export const Email = pattern( + Varchar, + /^[^@\s]+@[^@\s]+\.[^@\s]+$/ +);
27-27: Consider making OperatorClass extensibleThe enum
["A", "B", "C"]seems restrictive. Consider if this needs to be more extensible or if there should be documentation explaining what each class represents.
29-30: Add documentation for Distance unitsPlease add a comment specifying the unit of measurement for Distance (meters, kilometers?).
+// Distance in meters export const Distance = size(integer(), 0, 1_000_000);
44-44: Document the purpose of phone_truncPlease add documentation explaining the purpose and format of the truncated phone number.
37-47: Ensure proper PII handlingThis structure contains sensitive personal information (name, email, phone). Ensure that:
- Data is properly encrypted at rest
- Access to these fields is logged
- GDPR compliance is maintained
49-60: Document the 'unknown' status use casePlease add documentation explaining when the 'unknown' status is used, as it might lead to ambiguous state handling.
63-63: Document the index limit rationalePlease add a comment explaining why the index is limited to 0-19.
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (1)
11-17: Consider making the anomaly label more flexibleThe
labelproperty is currently hardcoded to"temporal_overlap_anomaly". This might be too restrictive if other types of anomalies need to be supported in the future.Consider using a union type or enum:
- label: "temporal_overlap_anomaly"; + label: AnomalyLabel;And define the types:
type AnomalyLabel = "temporal_overlap_anomaly" | "other_anomaly_types"; // or enum AnomalyLabel { TemporalOverlap = "temporal_overlap_anomaly", // other labels... }api/src/pdc/services/acquisition/actions/ListJourneyAction.ts (1)
Line range hint
37-52: Consider removing unsafe type assertion in status castingThe
as anytype assertion incastFromStatusEnum(status as any)bypasses type safety. Consider defining proper types for the status enum.- status: castFromStatusEnum(status as any), + status: castFromStatusEnum(status),Also, consider adding type definition for the status enum in the ListJourney DTO to ensure type safety at the parameter level.
api/src/pdc/services/apdf/commands/ExportCommand.ts (1)
62-63: Consider refactoring to ensure signature consistencyWhile extracting the signature to a constant is good for maintainability, we should ensure it stays in sync with the decorator's signature.
Consider this refactor to maintain consistency:
+const EXPORT_SIGNATURE = "apdf:export"; @command({ - signature: "apdf:export", + signature: EXPORT_SIGNATURE, description: "Export APDF", // ... }) export class ExportCommand implements CommandInterface { public async call(options: Options): Promise<string> { - const exportSignature = "apdf:export"; + const exportSignature = EXPORT_SIGNATURE;api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts (3)
Line range hint
19-36: Consider adding type safety to database queriesThe use of
anytype in the query result could be replaced with a proper interface type for better type safety.- const result = await this.connection.getClient().query<any>(query); + interface ApplicationRow { + uuid: string; + name: string; + owner_id: number; + owner_service: string; + permissions: string[]; + created_at: Date; + deleted_at: Date | null; + } + const result = await this.connection.getClient().query<ApplicationRow>(query);
Line range hint
40-69: Improve owner_id type handlingThe conditional check
owner_id && typeof owner_id !== "string"suggests potential type ambiguity. Consider making the type more explicit in the DTO.// in FindApplication.ts -owner_id: number | string | null; +owner_id: number | null;
Line range hint
118-125: Add error handling for missing configurationThe
applyDefaultPermissionsmethod should handle cases where the permissions configuration is missing or invalid.applyDefaultPermissions( application: ApplicationInterface, ): ApplicationInterface { + const defaultPermissions = this.config.get("permissions.application", []); + if (!Array.isArray(defaultPermissions)) { + throw new Error('Invalid permissions configuration'); + } return { ...application, - permissions: this.config.get("permissions.application", []), + permissions: defaultPermissions, }; }api/src/pdc/services/application/actions/RevokeApplicationAction.ts (1)
Line range hint
29-34: Handle potential errors from the repository methodCurrently, any exceptions thrown by
applicationRepository.revokewill propagate up. Consider adding error handling to manage potential exceptions and provide meaningful feedback.Apply this diff to include error handling:
public async handle( params: RevokeApplication, ): Promise<void> { - await this.applicationRepository.revoke({ - ...params, - owner_service: "operator", - }); + try { + await this.applicationRepository.revoke({ + ...params, + owner_service: "operator", + }); + } catch (error) { + // Handle specific errors if necessary + throw new Error('Failed to revoke application'); + } }api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts (2)
Line range hint
64-69: Use Direct Property Access for Improved Type SafetyIn the
validateParamsmethod, consider accessingstartandenddirectly from thejourneyobject instead of using thegetutility function. Sincejourneyis strongly typed asCreateJourney, direct access enhances type safety and code readability.Apply this diff:
protected async validateParams(journey: CreateJourney): Promise<void> { const now = new Date(); - const start = get(journey, "start.datetime") as Date; - const end = get(journey, "end.datetime") as Date; + const start = journey.start.datetime; + const end = journey.end.datetime; if (end > now || start > end) { throw new ParseErrorException("Journeys cannot happen in the future"); } }
96-96: Review Type Assertion onoperator_classThe type assertion
as OperatorClassonpayload.operator_classmay indicate that the property is not properly typed in theCreateJourneyDTO. Ensure thatoperator_classis correctly defined asOperatorClasswithin the DTO to avoid unnecessary type assertions and enhance type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (82)
api/src/lib/superstruct/index.ts(1 hunks)api/src/pdc/providers/carpool/interfaces/database/carpool.ts(1 hunks)api/src/pdc/providers/carpool/repositories/CarpoolStatusRepository.ts(1 hunks)api/src/pdc/providers/superstruct/ValidatorMiddleware.ts(2 hunks)api/src/pdc/providers/superstruct/shared/index.ts(1 hunks)api/src/pdc/providers/superstruct/shared/tz.ts(1 hunks)api/src/pdc/proxy/HttpTransport.ts(5 hunks)api/src/pdc/proxy/middlewares/serverTokenMiddleware.ts(1 hunks)api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts(2 hunks)api/src/pdc/services/acquisition/actions/CancelJourneyAction.ts(2 hunks)api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts(4 hunks)api/src/pdc/services/acquisition/actions/ListJourneyAction.ts(1 hunks)api/src/pdc/services/acquisition/actions/PatchJourneyAction.ts(5 hunks)api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts(2 hunks)api/src/pdc/services/acquisition/config/ajv.ts(0 hunks)api/src/pdc/services/acquisition/config/callContext.ts(0 hunks)api/src/pdc/services/acquisition/config/index.ts(0 hunks)api/src/pdc/services/acquisition/contracts/cancel.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/cancel.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionErrorInterface.ts(0 hunks)api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionInterface.ts(0 hunks)api/src/pdc/services/acquisition/contracts/create.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/create.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/list.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/list.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/patch.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/patch.schema.ts(0 hunks)api/src/pdc/services/acquisition/contracts/process.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/status.contract.ts(0 hunks)api/src/pdc/services/acquisition/contracts/status.schema.ts(0 hunks)api/src/pdc/services/acquisition/dto/CancelJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/CreateJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/ListJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/PatchJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/StatusJourney.ts(1 hunks)api/src/pdc/services/acquisition/dto/shared.ts(1 hunks)api/src/pdc/services/apdf/APDFServiceProvider.ts(2 hunks)api/src/pdc/services/apdf/actions/ExportAction.ts(3 hunks)api/src/pdc/services/apdf/actions/ListAction.ts(2 hunks)api/src/pdc/services/apdf/commands/ExportCommand.ts(3 hunks)api/src/pdc/services/apdf/contracts/export.contract.ts(0 hunks)api/src/pdc/services/apdf/contracts/export.schema.ts(0 hunks)api/src/pdc/services/apdf/contracts/list.contract.ts(0 hunks)api/src/pdc/services/apdf/contracts/list.schema.ts(0 hunks)api/src/pdc/services/apdf/dto/ExportApdf.ts(1 hunks)api/src/pdc/services/apdf/dto/ListApdf.ts(1 hunks)api/src/pdc/services/apdf/helpers/castExportParams.helper.ts(1 hunks)api/src/pdc/services/apdf/interfaces/APDFRepositoryProviderInterface.ts(1 hunks)api/src/pdc/services/apdf/interfaces/PolicySliceStatInterface.ts(1 hunks)api/src/pdc/services/apdf/interfaces/StorageRepositoryProviderInterface.ts(1 hunks)api/src/pdc/services/apdf/providers/APDFRepositoryProvider.ts(1 hunks)api/src/pdc/services/apdf/providers/CheckCampaign.ts(1 hunks)api/src/pdc/services/apdf/providers/StorageRepositoryProvider.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/BuildExcel.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/BuildExcel.unit.spec.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.integration.spec.ts(1 hunks)api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts(1 hunks)api/src/pdc/services/application/ApplicationServiceProvider.ts(2 hunks)api/src/pdc/services/application/actions/CreateApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/FindApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/ListApplicationAction.ts(1 hunks)api/src/pdc/services/application/actions/RevokeApplicationAction.ts(1 hunks)api/src/pdc/services/application/contracts/common/interfaces/ApplicationCreateResultInterface.ts(0 hunks)api/src/pdc/services/application/contracts/create.contract.ts(0 hunks)api/src/pdc/services/application/contracts/create.schema.ts(0 hunks)api/src/pdc/services/application/contracts/find.contract.ts(0 hunks)api/src/pdc/services/application/contracts/find.schema.ts(0 hunks)api/src/pdc/services/application/contracts/list.contract.ts(0 hunks)api/src/pdc/services/application/contracts/list.schema.ts(0 hunks)api/src/pdc/services/application/contracts/revoke.contract.ts(0 hunks)api/src/pdc/services/application/contracts/revoke.schema.ts(0 hunks)api/src/pdc/services/application/dto/CreateApplication.ts(1 hunks)api/src/pdc/services/application/dto/FindApplication.ts(1 hunks)api/src/pdc/services/application/dto/ListApplication.ts(1 hunks)api/src/pdc/services/application/dto/RevokeApplication.ts(1 hunks)api/src/pdc/services/application/interfaces/ApplicationRepositoryProviderInterface.ts(0 hunks)api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts(5 hunks)api/src/pdc/services/cee/CeeServiceProvider.ts(2 hunks)api/src/pdc/services/cee/actions/DeleteCeeAction.ts(2 hunks)api/src/pdc/services/cee/actions/FindCeeAction.ts(2 hunks)api/src/pdc/services/cee/actions/ImportCeeAction.integration.spec.ts(0 hunks)api/src/pdc/services/cee/actions/ImportCeeAction.ts(0 hunks)
⛔ Files not processed due to max files limit (28)
- api/src/pdc/services/cee/actions/ImportCeeIdentityAction.integration.spec.ts
- api/src/pdc/services/cee/actions/ImportCeeIdentityAction.ts
- api/src/pdc/services/cee/actions/RegisterCeeAction.integration.spec.ts
- api/src/pdc/services/cee/actions/RegisterCeeAction.ts
- api/src/pdc/services/cee/actions/SimulateCeeAction.integration.spec.ts
- api/src/pdc/services/cee/actions/SimulateCeeAction.ts
- api/src/pdc/services/cee/contracts/common/ceeSchema.ts
- api/src/pdc/services/cee/contracts/deleteApplication.contract.ts
- api/src/pdc/services/cee/contracts/deleteApplication.schema.ts
- api/src/pdc/services/cee/contracts/findApplication.contract.ts
- api/src/pdc/services/cee/contracts/findApplication.schema.ts
- api/src/pdc/services/cee/contracts/importApplication.contract.ts
- api/src/pdc/services/cee/contracts/importApplication.schema.ts
- api/src/pdc/services/cee/contracts/importApplicationIdentity.contract.ts
- api/src/pdc/services/cee/contracts/importApplicationIdentity.schema.ts
- api/src/pdc/services/cee/contracts/registerApplication.contract.ts
- api/src/pdc/services/cee/contracts/registerApplication.schema.ts
- api/src/pdc/services/cee/contracts/simulateApplication.contract.ts
- api/src/pdc/services/cee/contracts/simulateApplication.schema.ts
- api/src/pdc/services/cee/dto/DeleteApplication.ts
- api/src/pdc/services/cee/dto/FindApplication.ts
- api/src/pdc/services/cee/dto/RegisterApplication.ts
- api/src/pdc/services/cee/dto/SimulateApplication.ts
- api/src/pdc/services/cee/dto/shared.ts
- api/src/pdc/services/cee/interfaces/CeeApplicationInterface.ts
- api/src/pdc/services/cee/interfaces/CeeRepositoryProviderInterface.ts
- api/src/pdc/services/policy/contracts/common/schema/createJourneyCommon.ts
- api/src/pdc/services/policy/contracts/simulateOnFuture.schema.ts
💤 Files with no reviewable changes (32)
- api/src/pdc/services/acquisition/config/ajv.ts
- api/src/pdc/services/acquisition/config/callContext.ts
- api/src/pdc/services/acquisition/config/index.ts
- api/src/pdc/services/acquisition/contracts/cancel.contract.ts
- api/src/pdc/services/acquisition/contracts/cancel.schema.ts
- api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionErrorInterface.ts
- api/src/pdc/services/acquisition/contracts/common/interfaces/AcquisitionInterface.ts
- api/src/pdc/services/acquisition/contracts/create.contract.ts
- api/src/pdc/services/acquisition/contracts/create.schema.ts
- api/src/pdc/services/acquisition/contracts/list.contract.ts
- api/src/pdc/services/acquisition/contracts/list.schema.ts
- api/src/pdc/services/acquisition/contracts/patch.contract.ts
- api/src/pdc/services/acquisition/contracts/patch.schema.ts
- api/src/pdc/services/acquisition/contracts/process.contract.ts
- api/src/pdc/services/acquisition/contracts/status.contract.ts
- api/src/pdc/services/acquisition/contracts/status.schema.ts
- api/src/pdc/services/apdf/contracts/export.contract.ts
- api/src/pdc/services/apdf/contracts/export.schema.ts
- api/src/pdc/services/apdf/contracts/list.contract.ts
- api/src/pdc/services/apdf/contracts/list.schema.ts
- api/src/pdc/services/application/contracts/common/interfaces/ApplicationCreateResultInterface.ts
- api/src/pdc/services/application/contracts/create.contract.ts
- api/src/pdc/services/application/contracts/create.schema.ts
- api/src/pdc/services/application/contracts/find.contract.ts
- api/src/pdc/services/application/contracts/find.schema.ts
- api/src/pdc/services/application/contracts/list.contract.ts
- api/src/pdc/services/application/contracts/list.schema.ts
- api/src/pdc/services/application/contracts/revoke.contract.ts
- api/src/pdc/services/application/contracts/revoke.schema.ts
- api/src/pdc/services/application/interfaces/ApplicationRepositoryProviderInterface.ts
- api/src/pdc/services/cee/actions/ImportCeeAction.integration.spec.ts
- api/src/pdc/services/cee/actions/ImportCeeAction.ts
✅ Files skipped from review due to trivial changes (3)
- api/src/pdc/services/apdf/providers/excel/BuildExcel.ts
- api/src/pdc/services/apdf/providers/excel/BuildExcel.unit.spec.ts
- api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- api/src/lib/superstruct/index.ts
- api/src/pdc/providers/carpool/interfaces/database/carpool.ts
- api/src/pdc/providers/carpool/repositories/CarpoolStatusRepository.ts
- api/src/pdc/providers/superstruct/shared/index.ts
- api/src/pdc/providers/superstruct/shared/tz.ts
- api/src/pdc/proxy/HttpTransport.ts
- api/src/pdc/proxy/middlewares/serverTokenMiddleware.ts
- api/src/pdc/services/acquisition/actions/CancelJourneyAction.ts
- api/src/pdc/services/acquisition/actions/PatchJourneyAction.ts
- api/src/pdc/services/acquisition/dto/CancelJourney.ts
- api/src/pdc/services/acquisition/dto/ListJourney.ts
- api/src/pdc/services/acquisition/dto/PatchJourney.ts
- api/src/pdc/services/acquisition/dto/StatusJourney.ts
- api/src/pdc/services/apdf/actions/ExportAction.ts
- api/src/pdc/services/apdf/actions/ListAction.ts
- api/src/pdc/services/apdf/dto/ExportApdf.ts
- api/src/pdc/services/apdf/dto/ListApdf.ts
- api/src/pdc/services/apdf/interfaces/APDFRepositoryProviderInterface.ts
- api/src/pdc/services/apdf/interfaces/PolicySliceStatInterface.ts
- api/src/pdc/services/apdf/providers/APDFRepositoryProvider.ts
- api/src/pdc/services/apdf/providers/CheckCampaign.ts
- api/src/pdc/services/apdf/providers/excel/SlicesWorksheetWriter.ts
- api/src/pdc/services/application/actions/CreateApplicationAction.ts
- api/src/pdc/services/application/actions/FindApplicationAction.ts
- api/src/pdc/services/application/actions/ListApplicationAction.ts
- api/src/pdc/services/application/dto/CreateApplication.ts
- api/src/pdc/services/application/dto/FindApplication.ts
- api/src/pdc/services/application/dto/ListApplication.ts
- api/src/pdc/services/application/dto/RevokeApplication.ts
- api/src/pdc/services/cee/actions/DeleteCeeAction.ts
- api/src/pdc/services/cee/actions/FindCeeAction.ts
🔇 Additional comments (35)
api/src/pdc/services/apdf/interfaces/StorageRepositoryProviderInterface.ts (1)
2-2: Verify the impact of import path change
The import path for EnrichedApdfType has been updated from contracts to actions. Let's verify this change is consistent across the codebase.
api/src/pdc/providers/superstruct/ValidatorMiddleware.ts (3)
22-26: LGTM! Improved error handling and cleaner flow
The new validation approach with tuple destructuring provides clearer error handling and a more functional flow. The immediate error throwing makes the control flow more explicit.
22-22: Verify implications of the coerce option
The addition of { coerce: true } means the validator will attempt to coerce values to match the schema types. This could have unexpected consequences if not properly tested with various input types.
Consider documenting the coercion behavior in the API documentation to make it clear to API consumers how their input will be handled.
✅ Verification successful
Coercion option is safely implemented with proper validation
The implementation is safe for the following reasons:
- The middleware properly handles validation errors by throwing
InvalidParamsExceptionwith clear error messages - Test cases explicitly verify type validation (e.g., rejecting number for string field)
- The coercion is applied uniformly across all service providers through the centralized ValidatorMiddleware
- Validation schemas are explicitly defined for each endpoint in service providers
- Failed validations produce clear error messages that help API consumers identify type mismatches
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test cases and usages that might be affected by coercion
# Expected: Should find test cases covering type coercion scenarios
# Search for schema definitions that might be affected by coercion
rg -A 5 "superstruct\.(string|number|boolean|object|array)"
# Look for existing test cases related to validation
fd -g "*validator*.test.ts" -g "*validator*.spec.ts"
Length of output: 919
Script:
#!/bin/bash
# Let's examine the test file more thoroughly to understand the coercion testing
cat api/src/pdc/providers/superstruct/ValidatorMiddleware.unit.spec.ts
# Also search for any validation-related tests in the codebase
rg -l "validate.*test|test.*validate" --type ts
# Look for actual usage of the ValidatorMiddleware to understand the impact
rg -l "ValidatorMiddleware" --type ts
Length of output: 3684
Script:
#!/bin/bash
# Let's examine the actual validator middleware implementation to understand the coercion context
cat api/src/pdc/providers/superstruct/ValidatorMiddleware.ts
# Let's also check one of the service providers to see how validation schemas are defined
cat api/src/pdc/services/user/UserServiceProvider.ts
# And check if there are any existing validation errors in logs or issues
rg "Expected.*but received" --type ts
Length of output: 6710
11-11: Verify the implementation of the custom validate function
The introduction of a custom validation wrapper is a good practice for centralizing validation logic. However, we should ensure its implementation is robust and consistent.
api/src/pdc/services/cee/CeeServiceProvider.ts (3)
1-11: LGTM! Well-organized imports
The imports are logically grouped and follow consistent path conventions.
29-29: LGTM! Clean service provider implementation
The empty class body is appropriate as all configuration is handled by the decorator, following the framework's patterns.
Line range hint 15-28: Verify the impact of removed import actions
The removal of ImportCeeAction and ImportCeeIdentityAction could affect existing functionality. Let's verify there are no remaining dependencies on these actions.
✅ Verification successful
No impact from removed import actions
The verification shows that the removed ImportCeeAction and ImportCeeIdentityAction have no remaining dependencies in the codebase. While there is an import schema file (importApplication.schema.ts), it only defines types and schemas without any direct connection to the removed actions. The current service configuration in CeeServiceProvider.ts correctly reflects all active actions (Register, Simulate, Find, Delete) that are properly imported and used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dependencies on removed import actions
# Look for references to the removed actions in routes, tests, or client code
rg -l "ImportCeeAction|ImportCeeIdentityAction"
# Check for any import-related API endpoints that might be affected
fd -e ts -e json -x rg -l "import.*cee" {}
Length of output: 385
Script:
#!/bin/bash
# Let's check the content of importApplication.schema.ts to understand its purpose
cat ./shared/cee/importApplication.schema.ts
# Check the content of Kernel.ts to see if it has any direct dependencies
cat ./api/src/pdc/proxy/Kernel.ts
# Look for any routes or configurations that might reference import functionality
rg -A 5 "import.*cee" --type ts
Length of output: 7207
api/src/pdc/services/application/ApplicationServiceProvider.ts (2)
Line range hint 1-28: LGTM! Clean and well-structured service provider implementation
The service provider is well organized with:
- Clear separation of concerns
- Proper dependency injection setup
- Consistent middleware configuration
- Well-defined handler registration
Line range hint 14-27: Verify validation middleware configuration across the application
The middleware configuration looks correct, but let's verify that other service providers are consistently using the same validation setup to ensure uniformity across the codebase.
✅ Verification successful
Validation middleware configuration is consistent across service providers
The verification shows that the validation middleware setup in ApplicationServiceProvider.ts follows the same pattern used consistently across all service providers in the codebase:
- All service providers use the same middleware configuration pattern:
middlewares: [...defaultMiddlewareBindings, [
"validate",
ValidatorMiddleware,
]]- The ValidatorMiddleware is imported from the same location in all services:
- Most services use:
@/pdc/providers/validator/index.ts - Some newer services use:
@/pdc/providers/superstruct/ValidatorMiddleware.ts
- The validation setup is consistently applied across all major services including:
- ApplicationServiceProvider
- UserServiceProvider
- PolicyServiceProvider
- TerritoryServiceProvider
- ObservatoryServiceProvider
- And many others
The middleware configuration in ApplicationServiceProvider.ts aligns with the codebase-wide validation strategy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other service providers use the same validation middleware setup
# Expected: Similar middleware configurations in other service providers
# Search for service provider decorators with middleware configurations
ast-grep --pattern '@serviceProvider({
$$$
middlewares: [...$_, ["validate", ValidatorMiddleware],$$$]
$$$
})'
# Search for any other validation middleware usage patterns
rg -l "ValidatorMiddleware" --type typescript
Length of output: 1248
Script:
#!/bin/bash
# Let's try a different approach to find service providers and their middleware configurations
# Find all service provider files
fd ServiceProvider.ts
# Search for ValidatorMiddleware usage with context
rg "ValidatorMiddleware" -A 3 -B 3
# Search for middleware configurations in service providers
rg "middlewares: \[" -A 5 -B 2
Length of output: 103994
api/src/pdc/services/apdf/APDFServiceProvider.ts (2)
30-30: LGTM! Clean and concise service provider implementation
The simplified service provider class correctly extends AbstractServiceProvider without unnecessary complexity. This aligns with the KISS principle and makes the code more maintainable.
Line range hint 1-30: Architectural Review: Validation Strategy Refactor
The refactoring from extension-based validation to middleware-based validation using superstruct is a positive architectural change that:
- Reduces complexity by removing the extension system
- Improves maintainability through centralized validation
- Leverages a well-established validation library (superstruct)
However, ensure that:
- All validation rules have been migrated to the new system
- The validation middleware handles all previous validation scenarios
- Error handling and error messages are consistent with the previous implementation
Let's verify the validation coverage:
✅ Verification successful
Validation Migration Successfully Verified
The codebase shows a complete and consistent migration to the new middleware-based validation system using superstruct:
- DTOs (ListApdf, ExportApdf) properly define their validation schemas using superstruct
- No traces of the old extension-based validation system remain
- Validation rules cover all essential fields including campaign_id, operator_id, territory_id, and export-specific parameters
- The validation middleware is correctly configured in the service provider
The architectural change has been implemented thoroughly and consistently across the APDF service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for validation schema definitions
rg -l "struct|superstruct" api/src/pdc/services/apdf/
# Look for any validation-related TODO or FIXME comments
rg -l "TODO.*validat|FIXME.*validat" api/src/pdc/services/apdf/
Length of output: 642
Script:
#!/bin/bash
# Let's check the validation schemas in the DTO files and actions
rg -A 10 "struct|superstruct" api/src/pdc/services/apdf/dto/
rg -A 10 "struct|superstruct" api/src/pdc/services/apdf/actions/
# Check for any old validation patterns that might have been missed
rg -l "validate|validation" api/src/pdc/services/apdf/ --type ts
# Look for extension-based validation (old system)
rg -l "extends.*Validator|@validator" api/src/pdc/services/apdf/
Length of output: 3775
api/src/pdc/services/apdf/helpers/castExportParams.helper.ts (2)
3-3: Verify the impact of type change across the codebase.
The change from ParamsInterface to ExportApdf type suggests a move towards more specific typing, which is good for type safety. However, we should verify that all callers of this function have been updated accordingly.
Also applies to: 6-6
✅ Verification successful
Let me analyze the search results to verify the impact of the type change.
Based on the analysis of the code, I can now generate the final response:
Type change is safe and properly implemented
The change from ParamsInterface to ExportApdf is a safe refactoring as:
- The
ExportApdftype is properly defined with all necessary fields using superstruct - All callers of
castExportParamsare already using the correct type:ExportCommand.tsexplicitly casts toExportApdfExportAction.tsreceives params of the correct type through API validation
The implementation handles all edge cases of date parameters and provides proper defaults, making it a robust and type-safe change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of castExportParams to verify they've been updated
# to use the new ExportApdf type
# Search for function calls
echo "Searching for castExportParams usage:"
rg -p "castExportParams\(" --type ts
# Search for ParamsInterface references that might need updating
echo -e "\nSearching for remaining ParamsInterface references:"
rg -p "ParamsInterface" --type ts
Length of output: 65576
Script:
#!/bin/bash
# Let's examine the castExportParams function implementation and its usage
# First, check the implementation
echo "Checking castExportParams implementation:"
cat api/src/pdc/services/apdf/helpers/castExportParams.helper.ts
# Then, check the ExportApdf type definition
echo -e "\nChecking ExportApdf type definition:"
cat api/src/pdc/services/apdf/dto/ExportApdf.ts
Length of output: 2026
Line range hint 41-44: Consider timezone edge cases.
The function mixes local time operations with zoned time conversions. This could lead to unexpected behavior around DST transitions or when processing dates across different timezones.
Consider applying timezone conversion consistently throughout the function:
return {
- start_date: fromZonedTime(start, params.format?.tz),
- end_date: fromZonedTime(end, params.format?.tz),
+ start_date: fromZonedTime(start, params.format?.tz ?? 'UTC'),
+ end_date: fromZonedTime(end, params.format?.tz ?? 'UTC'),
};api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts (2)
40-40: LGTM: Simplified service provider class
The removal of the validator extension and related properties has appropriately simplified the service provider class, aligning with the new validation strategy.
7-7: Verify the ValidatorMiddleware integration
The validation strategy has been centralized through the ValidatorMiddleware. Let's verify that all necessary validation rules are properly configured in the new middleware.
Also applies to: 28-30
✅ Verification successful
Let me gather more information about the validation setup to ensure completeness of the verification.
Let me check one more aspect to verify how the validation is implemented in the actions.
Let me check one final aspect to understand how the validation is configured in the middleware setup.
Based on the gathered information, I can now provide the final response:
ValidatorMiddleware integration is properly configured
The validation setup has been correctly implemented through the ValidatorMiddleware:
- The middleware is properly registered in the service provider with the "validate" binding
- ValidatorMiddleware implements the standard middleware interface and handles validation using superstruct
- The middleware is included in the default middleware bindings array
- The implementation shows proper error handling and parameter validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ValidatorMiddleware implementation and validation rules
# Check the ValidatorMiddleware implementation
rg -A 10 "class ValidatorMiddleware" api/src/pdc/providers/superstruct/
# Look for validation rules or schemas that might be used by the middleware
ast-grep --pattern 'validate($$$)' api/src/pdc/providers/superstruct/
Length of output: 1243
Script:
#!/bin/bash
# Check how ValidatorMiddleware is used in the AcquisitionServiceProvider
rg -A 5 "ValidatorMiddleware" api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts
# Look for validation schemas in the acquisition service
rg -l "schema" api/src/pdc/services/acquisition/
# Check if there are any other validation-related imports or usage
ast-grep --pattern 'import { $_ } from "$_/superstruct/$_"'
Length of output: 770
Script:
#!/bin/bash
# Check the implementation of journey actions to see how they use validation
rg -A 5 "class.*Action" api/src/pdc/services/acquisition/actions/
# Look for schema definitions in the actions
ast-grep --pattern 'const schema = $_' api/src/pdc/services/acquisition/actions/
# Check for any validator decorator usage
rg -A 2 "@validator" api/src/pdc/services/acquisition/actions/
Length of output: 3722
Script:
#!/bin/bash
# Check the middleware configuration in the service provider
rg -B 5 -A 5 "middlewares:" api/src/pdc/services/acquisition/AcquisitionServiceProvider.ts
# Look for schema definitions in the validator middleware directory
rg -l "schema" api/src/pdc/providers/superstruct/
# Check for any validation-related types or interfaces
ast-grep --pattern 'interface $_Validator' api/src/pdc/providers/superstruct/
Length of output: 505
api/src/pdc/services/acquisition/dto/CreateJourney.ts (3)
50-55: 🛠️ Refactor suggestion
Consider constraining payment types
The type field is currently unconstrained. Consider using an enum to restrict it to valid payment types to prevent invalid values.
- type: Varchar,
+ type: enums(["cash", "card", "transfer"]), // Add valid payment types64-77:
Replace unknown() type with proper CarpoolIncentiveCounterpart structure
The incentive_counterparts field should use the existing CarpoolIncentiveCounterpart type from interfaces/common.ts which has a well-defined structure:
target: IncentiveCounterpartTarget (enum: driver/passenger)siret: Siretamount: Financial
This will provide proper type safety and validation.
1-79: Verify security-related validations
Since this schema handles sensitive data and financial transactions, let's verify if we have all necessary security validations in place.
✅ Verification successful
Schema validation and security measures are properly implemented
The verification reveals robust security measures in place:
- Input validation through superstruct with strict type checking
- Proper sanitization through the
sanitizeKeywordimplementation - Strict bounds checking for sensitive fields (Amount: 0-100,000, Distance: 0-1,000,000)
- Pattern validation for identifiers and emails
- Size limits on arrays (payments: 0-20, incentives: 0-20)
- Proper validation middleware that coerces and validates all inputs
The schema implements comprehensive security controls for handling sensitive data and financial transactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for security-related validations and sanitizations
echo "Checking for input sanitization..."
rg -l "sanitize|escape|validate" --type ts
echo "Checking for security-related types..."
ast-grep --pattern 'interface $_Security {
$$$
}'
echo "Checking for security middleware usage..."
rg -l "security|auth|sanitize" --glob "middleware/*.ts"
Length of output: 13843
Script:
#!/bin/bash
# Let's check for validation and sanitization specifically in the acquisition service and related files
echo "Checking validation in acquisition service..."
rg -A 5 "validate|sanitize" api/src/pdc/services/acquisition/
# Check for security middleware in the acquisition flow
echo "Checking security middleware..."
rg -l "auth|security" api/src/pdc/services/acquisition/
# Check for specific validation of sensitive fields
echo "Checking sensitive field validations..."
ast-grep --pattern 'const $_ = object({
$$$
identity: $_,
$$$
})'
# Check for payment validation patterns
echo "Checking payment validations..."
ast-grep --pattern 'const Payment = object({
$$$
})'
Length of output: 9783
Script:
#!/bin/bash
# Let's check the implementation of the base types used in the schema
echo "Checking Amount type implementation..."
rg -A 5 "export const Amount =" api/src/pdc/services/acquisition/dto/
echo "Checking shared type implementations..."
cat api/src/pdc/services/acquisition/dto/shared.ts
# Check for any security-related middleware in the validation chain
echo "Checking validation middleware..."
rg -A 10 "class ValidatorMiddleware" api/src/pdc/providers/superstruct/ValidatorMiddleware.ts
# Check for any sanitization in the validator
echo "Checking sanitization in validator..."
rg -A 5 "sanitize" api/src/pdc/providers/validator/
Length of output: 9564
api/src/pdc/services/acquisition/dto/shared.ts (4)
1-16: LGTM! Clean import organization
The imports and re-exports are well structured and purposeful.
31-35: LGTM! Well-structured geographical point with timestamp
The structure effectively combines timestamp and coordinates using previously defined validators.
68-69: LGTM! Reasonable pagination constraints
The pagination limits are well-defined and reasonable.
25-25: Verify ExternalId pattern usage
The pattern /^[0-9A-Za-z_-]{1,256}$/ allows underscores and hyphens. Please verify this aligns with all systems consuming this ID.
✅ Verification successful
Let me analyze the usage of ExternalId pattern in the codebase to verify its alignment with consuming systems.
Let me search for more specific patterns to understand the validation and usage.
Let me fix the syntax and try again.
Based on the analysis of the codebase, I can now provide a final response:
ExternalId pattern is correctly defined and widely used
The pattern /^[0-9A-Za-z_-]{1,256}$/ for ExternalId is well-aligned with the systems consuming it, as evidenced by:
- It's used consistently across multiple operator integrations (Klaxit, Karos, BlaBlaCar Daily, Mobicoop, etc.)
- The pattern is used for various ID types including operator_user_id, operator_journey_id, and operator_trip_id
- Test cases show successful usage with UUIDs and alphanumeric IDs containing hyphens and underscores
- No validation errors or issues related to this pattern were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ExternalId usage patterns
rg -A 2 "operator_user_id|ExternalId" --type ts
Length of output: 24558
Script:
#!/bin/bash
# Search for actual values of operator_journey_id, operator_trip_id, and operator_user_id in tests
rg -g '*.spec.ts' -g '*.test.ts' 'operator_(journey|trip|user)_id.*=.*["'\''].*["'\'']' --multiline
# Search for validation errors related to these IDs
rg 'ExternalId.*invalid|invalid.*ExternalId'
Length of output: 1132
Script:
#!/bin/bash
# Search for test cases and validation patterns
rg -g '*.spec.ts' -g '*.test.ts' 'operator.*id.*["\'][-_A-Za-z0-9]+["\']' -B 2 -A 2
# Search for validation error messages and schema definitions
rg 'pattern.*ExternalId|ExternalId.*pattern' -B 2 -A 2
# Search for actual validation errors in logs or tests
rg -g '*.spec.ts' -g '*.test.ts' 'invalid.*id|validation.*failed' -B 2 -A 2
Length of output: 116
Script:
#!/bin/bash
# Search for test cases and validation patterns
rg -g "*.spec.ts" -g "*.test.ts" "operator.*id.*[-_A-Za-z0-9]+" -B 2 -A 2
# Search for pattern definitions and usages
rg "ExternalId.*=.*pattern" -B 2 -A 2
# Search for validation related code
rg -g "*.ts" "validate.*ExternalId|invalid.*id" -B 2 -A 2
Length of output: 106076
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (3)
5-9: LGTM! Clean import structure
The new imports properly support the transition to stronger typing with DTOs and structured validation.
28-34: LGTM! Clean middleware configuration
The handler configuration is well-structured and properly integrates the new StatusJourney validation.
Line range hint 44-64: Remove 'any' type assertions for better type safety
The use of type assertions with 'any' bypasses TypeScript's type checking system, which could lead to runtime errors.
Consider this safer implementation:
- const status = castToStatusEnum(result.status) as any;
+ const status: Infer<typeof JourneyStatus> = castToStatusEnum(result.status);
return {
operator_journey_id,
status,
created_at: result.created_at,
fraud_error_labels: result.fraud.map((f) => f.label),
- anomaly_error_details: result.anomaly as any,
+ anomaly_error_details: result.anomaly as AnomalyErrorDetails[],
terms_violation_details: result.terms.map((f) => f.label),
};Additionally, consider adding runtime type validation for the anomaly data to ensure it matches the AnomalyErrorDetails interface.
api/src/pdc/services/acquisition/actions/ListJourneyAction.ts (3)
16-23: LGTM! Well-structured handler configuration
The handler is properly configured with:
- Correct service and method identification
- Appropriate permission middleware for operator scope
- Type-safe validation using the new ListJourney DTO
Line range hint 53-69: LGTM! Robust date validation implementation
The date validation logic is well-implemented with:
- Proper chronological order check
- Future date prevention
- Reasonable 90-day historical limit
- Clear error messages
11-13: Verify completeness of ResultInterface type definition
The interface seems overly minimal. Verify if the actual data returned by CarpoolStatusService.findBy() contains only the operator_journey_id field.
✅ Verification successful
ResultInterface type definition is correct and complete
The ResultInterface type definition matches exactly with the return type of CarpoolStatusService.findBy() method, which returns Array<{ operator_journey_id: string }>. The implementation in the service confirms that this is the only field returned by the underlying repository method getOperatorJourneyIdByStatus().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the return type or implementation of findBy method
ast-grep --pattern 'class CarpoolStatusService {
$$$
findBy($$$) {
$$$
}
$$$
}'
Length of output: 93
Script:
#!/bin/bash
# Let's try to find any references to CarpoolStatusService in the codebase
rg -l "CarpoolStatusService"
# Also search for any similar service that might handle journey status
rg -l "JourneyStatus|StatusService"
# Search for the actual usage of findBy in ListJourneyAction
rg -A 5 "findBy" api/src/pdc/services/acquisition/actions/ListJourneyAction.ts
Length of output: 871
Script:
#!/bin/bash
# Let's check the implementation of CarpoolStatusService
cat api/src/pdc/providers/carpool/providers/CarpoolStatusService.ts
# Also check the ListJourney DTO to understand the expected structure
cat api/src/pdc/services/acquisition/dto/ListJourney.ts
# And check shared DTOs that might contain relevant types
cat api/src/pdc/services/acquisition/dto/shared.ts
Length of output: 4403
api/src/pdc/services/apdf/commands/ExportCommand.ts (2)
6-6: LGTM: Import addition is well-placed
The new import for ExportApdf DTO follows the project's import conventions and aligns with the type changes.
94-94: Verify ExportApdf type compatibility
The type assertion and destructuring suggest that start_date and end_date are required fields in the ExportApdf type. Let's verify the type structure and its usage.
✅ Verification successful
Type assertion is valid and safe
The type assertion is correct as the ExportApdf schema shows that query.date.start and query.date.end are optional fields, and the castExportParams function properly handles this by checking their existence before destructuring. The function returns a normalized object with start_date and end_date as Date objects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ExportApdf type structure and its usage
# Check ExportApdf type definition
ast-grep --pattern 'export (type|interface) ExportApdf'
# Check other usages of castExportParams
rg -A 5 'castExportParams\(' --type typescript
Length of output: 141
Script:
#!/bin/bash
# Let's try different approaches to find the type definition and usage
# Search for ExportApdf in all files
rg "ExportApdf"
# Search for castExportParams function
rg "castExportParams"
# Find all TypeScript files that might contain these definitions
fd "\.ts$"
Length of output: 115954
Script:
#!/bin/bash
# Let's check the ExportApdf type definition and castExportParams function
# Search for ExportApdf type/schema definition
rg -A 10 "export (const|type|interface) ExportApdf" api/src/pdc/services/apdf/dto/ExportApdf.ts
# Search for castExportParams function implementation
rg -A 10 "export function castExportParams" api/src/pdc/services/apdf/helpers/castExportParams.helper.ts
Length of output: 872
api/src/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts (2)
Line range hint 1-125: Security review: Implementation follows secure practices
The implementation demonstrates good security practices:
- SQL injection prevention through parameterized queries
- Proper error handling for critical operations
- Permission management through configuration
4-11: Verify architectural decision to remove interface implementation
The removal of ApplicationRepositoryProviderInterface implementation might impact type safety and interface segregation. Consider if this aligns with the project's architectural principles.
api/src/pdc/services/application/actions/RevokeApplicationAction.ts (2)
29-30: Ensure method signature changes are propagated
Changing the method signature of handle might affect other parts of the codebase that extend or use RevokeApplicationAction. Verify that all subclasses and calls are updated accordingly.
Run the following script to identify usages of RevokeApplicationAction and ensure compatibility:
✅ Verification successful
Let me gather more information about the method signature and its potential implementations.
Let me try a different approach to gather information about the implementation and usage.
Method signature change is safe
The method signature change in RevokeApplicationAction is safe as:
- The class is not extended by any other classes in the codebase
- The action is only used as a dependency injection in
ApplicationServiceProvider - The
handlemethod implementation follows the baseAbstractActionpattern - The method is only called through the service provider's middleware pipeline, which validates the input using the
RevokeApplicationDTO
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of RevokeApplicationAction
# Search for references to RevokeApplicationAction in the codebase
rg -t ts 'RevokeApplicationAction' -A 3 -B 3
Length of output: 2195
Script:
#!/bin/bash
# Search for the handle method implementation in RevokeApplicationAction
ast-grep --pattern 'class RevokeApplicationAction {
$$$
handle($$$) {
$$$
}
$$$
}'
# Search for any class that might extend RevokeApplicationAction
rg -t ts 'extends.*RevokeApplicationAction'
# Search for any direct usage/calls to handle method on RevokeApplicationAction
rg -t ts 'revokeApplication.*handle\(' -i
Length of output: 186
Script:
#!/bin/bash
# Get the full content of RevokeApplicationAction to understand the implementation
cat api/src/pdc/services/application/actions/RevokeApplicationAction.ts
# Search for any imports of RevokeApplicationAction and potential usage
rg -t ts -B 2 -A 4 'from.*RevokeApplicationAction'
# Search for potential controller or route handler using this action
rg -t ts 'RevokeApplication' -B 2 -A 4
Length of output: 8536
18-18:
Validate ownership in the middleware
Ensure that the copyFromContextMiddleware and hasPermissionByScopeMiddleware adequately validate the operator's ownership over the application being revoked to prevent unauthorized access.
Run the following script to check if ownership validation is correctly implemented:
✅ Verification successful
Ownership validation is properly implemented
The ownership validation is correctly implemented through a combination of middlewares:
copyFromContextMiddleware("call.user.operator_id", "owner_id")copies the operator's ID from the context to ensure the request is associated with the correct ownerhasPermissionByScopeMiddlewarevalidates that either:- The user has the global permission "operator.application.revoke"
- The user's operator_id matches the owner_id parameter and has the scoped permission
The implementation is secure as it:
- Enforces permission checks
- Validates ownership by comparing operator_id with owner_id
- Throws ForbiddenException for unauthorized access
- Has comprehensive test coverage for various scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ownership validation is enforced in the middlewares
# Search for usage of copyFromContextMiddleware and related ownership checks
rg 'copyFromContextMiddleware\("call\.user\.operator_id", "owner_id"\)' -A 5 api/src/pdc/
# Check for tests related to ownership validation
rg 'should enforce ownership' -A 3 api/tests/pdc/
Length of output: 2666
Script:
#!/bin/bash
# Let's verify the implementation of hasPermissionByScopeMiddleware and its ownership validation
# Check the implementation of hasPermissionByScopeMiddleware
ast-grep --pattern 'function hasPermissionByScopeMiddleware' -A 20
# Check for any test files related to application actions
fd test.*\.ts$ api/src/pdc/services/application/
# Check the RevokeApplication implementation to understand the validation flow
ast-grep --pattern 'class RevokeApplication' -A 30
Length of output: 173
Script:
#!/bin/bash
# Let's try a different approach to find the middleware and validation implementations
# Search for the middleware file that might contain hasPermissionByScopeMiddleware
fd middleware api/src/pdc/
# Look for RevokeApplication class implementation with ripgrep
rg "class RevokeApplication" -A 30
# Search for any permission or scope validation related code
rg "hasPermissionByScope" -A 5
Length of output: 20762
Script:
#!/bin/bash
# Let's examine the HasPermissionByScopeMiddleware implementation
# Check the implementation file
cat api/src/pdc/providers/middleware/HasPermissionByScope/HasPermissionByScopeMiddleware.ts
# Look for the middleware test file
cat api/src/pdc/providers/middleware/HasPermissionByScope/HasPermissionByScopeMiddleware.unit.spec.ts
# Check the RevokeApplication DTO to understand what's being validated
cat api/src/pdc/services/application/dto/RevokeApplication.ts
Length of output: 8686
api/src/pdc/services/acquisition/actions/CreateJourneyAction.ts (1)
Line range hint 90-125: Verify Validation of Nested Properties to Prevent Runtime Errors
In the convertPayloadToRequest method, nested properties like payload.driver.identity.phone are accessed directly. Confirm that the CreateJourney DTO and the validation middleware ensure all required nested properties are present and properly validated. This prevents potential runtime errors due to undefined or null values.
| @@ -1,5 +1,5 @@ | |||
| import { S3ObjectList } from "@/pdc/providers/storage/index.ts"; | |||
| import { EnrichedApdfType } from "../contracts/list.contract.ts"; | |||
| import { EnrichedApdfType } from "../actions/ListAction.ts"; | |||
|
|
|||
| // TODO fix | |||
| export type SerializedPolicyInterface = any; | |||
There was a problem hiding this comment.
Fix TODO and remove any type usage
The SerializedPolicyInterface is currently typed as any, which bypasses TypeScript's type checking. This could lead to runtime errors and makes the code less maintainable.
Would you like me to help define a proper interface for SerializedPolicyInterface? This would improve type safety and code maintainability.
| import { ValidatorMiddleware } from "@/pdc/providers/superstruct/ValidatorMiddleware.ts"; | ||
| import { CeeRepositoryProvider } from "./providers/CeeRepositoryProvider.ts"; |
There was a problem hiding this comment.
💡 Codebase verification
Validation strategy needs to be unified across services
The verification reveals inconsistent validation implementations across the codebase:
- Most services are still using the old
ValidatorExtensionfrom@/pdc/providers/validator/index.ts - Only a few services (cee, application, apdf, acquisition) have migrated to the new
ValidatorMiddlewarefrom@/pdc/providers/superstruct/ValidatorMiddleware.ts - Both old and new validation implementations coexist in the codebase
This architectural inconsistency should be addressed:
- All services should migrate to use
@/pdc/providers/superstruct/ValidatorMiddleware.ts - Remove the deprecated
ValidatorExtensionimplementation - Update service providers to remove
ValidatorExtensionfrom their extensions array
🔗 Analysis chain
Verify the new validation strategy implementation
The transition from ValidatorExtension to ValidatorMiddleware represents a significant architectural change. Let's verify the implementation is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the validation strategy implementation
# Check for any remaining ValidatorExtension usage that should be migrated
rg -l "ValidatorExtension"
# Check the new ValidatorMiddleware usage pattern
ast-grep --pattern 'class $_ implements ValidatorMiddleware {
$$$
}'
# Look for validation-related configurations in other service providers
rg -A 5 "ValidatorMiddleware" --type ts
Length of output: 21811
| import { DataRepositoryProvider } from "./providers/APDFRepositoryProvider.ts"; | ||
| import { StorageRepositoryProvider } from "./providers/StorageRepositoryProvider.ts"; | ||
|
|
||
| import { ValidatorMiddleware } from "@/pdc/providers/superstruct/ValidatorMiddleware.ts"; |
There was a problem hiding this comment.
💡 Codebase verification
Migration to superstruct validation is incomplete across the codebase
The verification reveals an inconsistent validation approach across the services:
- Several services (user, territory, policy, etc.) are still using the old
ValidatorExtensionpattern - Only a few services (apdf, acquisition, cee, application) have migrated to the new
ValidatorMiddlewarefrom superstruct - Some services are importing both
ValidatorExtensionandValidatorMiddlewarefrom the old validator package
This indicates that the architectural change to superstruct validation is in progress but not fully implemented across all services, which could lead to inconsistent validation behavior.
🔗 Analysis chain
Verify the migration to superstruct validation
The switch to ValidatorMiddleware from superstruct represents a significant architectural change in the validation strategy.
Let's verify the validation migration is complete:
Let me gather more information about the validation patterns to understand the migration status better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining old validation patterns that might have been missed
rg -l "ValidatorExtension|listBinding|exportBinding" api/src/pdc/services/
# Check for new validation patterns to ensure consistent adoption
ast-grep --pattern 'import { ValidatorMiddleware } from "@/pdc/providers/superstruct/ValidatorMiddleware.ts"'
Length of output: 1550
Script:
#!/bin/bash
# Check the actual usage of ValidatorExtension in the files that still contain it
rg "ValidatorExtension" -A 5 api/src/pdc/services/
# Check how ValidatorMiddleware is being used in the migrated files
rg "ValidatorMiddleware" -A 5 api/src/pdc/services/apdf/APDFServiceProvider.ts
Length of output: 11234
| @@ -8,7 +8,7 @@ import { | |||
| S3ObjectList, | |||
| S3StorageProvider, | |||
| } from "@/pdc/providers/storage/index.ts"; | |||
| import { EnrichedApdfType } from "../contracts/list.contract.ts"; | |||
| import { EnrichedApdfType } from "../actions/ListAction.ts"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Type definition exists but appears to be duplicated
The verification reveals that EnrichedApdfType is defined in two locations:
shared/apdf/list.contract.tsapi/src/pdc/services/apdf/actions/ListAction.ts
Both definitions are identical, suggesting potential type duplication that should be addressed:
{
signed_url: string;
key: string;
size: number;
operator_id: number;
campaign_id: number;
datetime: Date;
name: string;
}🔗 Analysis chain
LGTM! Verify type definition in new location.
The import path change maintains type safety while following the apparent refactoring pattern of moving types from contracts to actions.
Let's verify the type definition exists in the new location:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EnrichedApdfType definition exists in the new location
# and check for any other usages that might need updating
# Check if the type exists in the new location
ast-grep --pattern 'type EnrichedApdfType = {
$$$
}'
# Look for any potentially missed imports that still use the old path
rg "from.*contracts/list\.contract"
Length of output: 3078
| ], | ||
| }) | ||
| export class RevokeApplicationAction extends AbstractAction { | ||
| constructor( | ||
| private applicationRepository: ApplicationRepositoryProviderInterfaceResolver, | ||
| private applicationRepository: ApplicationPgRepositoryProvider, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use interface for dependency injection in the constructor
Updating the constructor to depend on the interface allows for easier testing and future extension.
Apply this diff to update the constructor parameter:
- private applicationRepository: ApplicationPgRepositoryProvider,
+ private applicationRepository: ApplicationRepositoryProviderInterfaceResolver,Committable suggestion skipped: line range outside the PR's diff.
| import { alias } from "../contracts/revoke.schema.ts"; | ||
| import { ApplicationRepositoryProviderInterfaceResolver } from "../interfaces/ApplicationRepositoryProviderInterface.ts"; | ||
| import { RevokeApplication } from "@/pdc/services/application/dto/RevokeApplication.ts"; | ||
| import { ApplicationPgRepositoryProvider } from "@/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject repository via interface rather than concrete class
To adhere to the Dependency Inversion Principle and promote flexibility, consider injecting the repository via its interface ApplicationRepositoryProviderInterfaceResolver instead of the concrete class ApplicationPgRepositoryProvider.
Apply this diff to update the import:
- import { ApplicationPgRepositoryProvider } from "@/pdc/services/application/providers/ApplicationPgRepositoryProvider.ts";
+ import { ApplicationRepositoryProviderInterfaceResolver } from "@/pdc/services/application/providers/ApplicationRepositoryProviderInterfaceResolver.ts";Committable suggestion skipped: line range outside the PR's diff.
| copyFromContextMiddleware("call.user.operator_id", "owner_id"), | ||
| hasPermissionByScopeMiddleware(undefined, [ | ||
| "operator.application.revoke", | ||
| "call.user.operator_id", | ||
| "owner_id", | ||
| ]), |
There was a problem hiding this comment.
💡 Codebase verification
Permission scope mismatch detected
The permission scope operator.application.revoke used in the middleware doesn't match the defined permissions in the configuration. According to the permissions configuration, the correct permission scope should be application.revoke, which is restricted to operator.admin role.
- In
api/src/pdc/services/user/config/permissions.ts, the permission is defined as:"application.revoke": ["operator.admin"]
🔗 Analysis chain
Verify the permission scope definition
Ensure that the permission scope operator.application.revoke used in hasPermissionByScopeMiddleware is properly defined in the permissions configuration and aligns with the permissions matrix.
Run the following script to confirm the permission scope definition:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'operator.application.revoke' is defined in permission configurations
# Search for 'operator.application.revoke' in permissions configuration files
rg 'operator\.application\.revoke' api/src/pdc/services/**/config/permissions.ts
Length of output: 139
Script:
#!/bin/bash
# Let's try a broader search for permission configurations and application-related permissions
# First, let's find all permission configuration files
echo "=== Permission Configuration Files ==="
fd "permissions.ts$"
# Then, let's search for any application-related permissions
echo -e "\n=== Application-Related Permissions ==="
rg -A 3 "application" -g "permissions.ts"
# Also search for operator permissions pattern
echo -e "\n=== Operator Permissions ==="
rg -A 3 "operator\." -g "permissions.ts"
Length of output: 12247
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation