ENG-602 FhirToCsv lambda calls the transform one directly #4217
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ref eng-602 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
WalkthroughThe changes introduce secure handling of Snowflake credentials for the FHIR-to-CSV Lambda workflow. Secrets are now passed via AWS Secrets Manager, with new environment variable configurations and runtime secret retrieval. Related infrastructure code is updated to support secret injection, and internal function calls are refactored for improved separation between API and direct invocation layers. Changes
Sequence Diagram(s)sequenceDiagram
participant APIStack
participant AnalyticsPlatformsNestedStack
participant AWSSecretsManager
participant FhirToCsvLambda
participant FhirToCsvDirect
APIStack->>AnalyticsPlatformsNestedStack: Instantiate with secrets
AnalyticsPlatformsNestedStack->>FhirToCsvLambda: Configure with SNOWFLAKE_CREDS_SECRET_NAME env var
FhirToCsvLambda->>AWSSecretsManager: getSecret(secretName)
AWSSecretsManager-->>FhirToCsvLambda: Return Snowflake credentials
FhirToCsvLambda->>FhirToCsvDirect: Set process.env.SNOWFLAKE_CREDS and invoke processing
FhirToCsvDirect-->>FhirToCsvLambda: Process result
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error code ERR_SSL_WRONG_VERSION_NUMBER 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/lambdas/src/analytics-platform/fhir-to-csv.ts (1)
30-35: Secure secret retrieval with proper validation.The implementation correctly:
- Fetches the secret asynchronously from AWS Secrets Manager
- Validates the secret exists and is not empty
- Fails fast with a clear error message if secret is missing
- Sets the environment variable at runtime before processing
The TODO comment about centralizing the environment variable name is valid technical debt.
Consider extracting the environment variable name to a shared constant to address the TODO:
// In a shared constants file export const SNOWFLAKE_CREDS_ENV_VAR = "SNOWFLAKE_CREDS"; // Then use it here process.env[SNOWFLAKE_CREDS_ENV_VAR] = creds;packages/infra/lib/analytics-platform/analytics-platform-stack.ts (1)
269-273: Improve variable naming for clarityThe secret retrieval logic is correct, but the variable name
snowflakeSecretEnvVarNameis misleading since it's used as a key to lookup the secret object, not an environment variable name.- const snowflakeSecretEnvVarName = "SNOWFLAKE_CREDS"; - const secret = ownProps.secrets[snowflakeSecretEnvVarName]; + const snowflakeSecretKey = "SNOWFLAKE_CREDS"; + const secret = ownProps.secrets[snowflakeSecretKey]; if (!secret) { - throw new Error(`${snowflakeSecretEnvVarName} secret not found`); + throw new Error(`${snowflakeSecretKey} secret not found`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/command/analytics-platform/api/start-fhir-to-csv-transform.ts(1 hunks)packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts(2 hunks)packages/infra/lib/analytics-platform/analytics-platform-stack.ts(7 hunks)packages/infra/lib/api-stack.ts(2 hunks)packages/lambdas/src/analytics-platform/fhir-to-csv.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursorrules
**/*.{ts,tsx}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .cursorrules
**/*.ts
Instructions used from:
Sources:
⚙️ CodeRabbit Configuration File
🧠 Learnings (6)
📓 Common learnings
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: leite08
PR: metriport/metriport#3817
File: .github/workflows/deploy-staging.yml:206-214
Timestamp: 2025-05-12T18:29:48.116Z
Learning: In the Metriport CI/CD workflows, the IHE stack deployment is intentionally triggered by general infrastructure changes (infra-lambdas) rather than having its own dedicated change detection, as it needs to stay in sync with other infrastructure components.
Learnt from: leite08
PR: metriport/metriport#4215
File: packages/core/src/command/analytics-platform/config.ts:4-13
Timestamp: 2025-07-20T01:18:28.071Z
Learning: User leite08 prefers that code improvement suggestions (like consolidating duplicated schemas) be made on feature PRs rather than release PRs. Such improvements should be deferred to follow-up PRs during the release cycle.
Learnt from: leite08
PR: metriport/metriport#0
File: :0-0
Timestamp: 2025-03-05T18:43:30.389Z
Learning: For backmerge PRs at metriport/metriport, only verify two things: (1) that the source branch is `master` and destination branch is `develop`, and (2) that there's a link to at least one PR (usually a "patch" PR) in the description. No need for detailed review comments or updates to the PR description unless there's an issue with these criteria.
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/command/get-bundle-by-resource-type.ts:31-37
Timestamp: 2025-06-05T14:09:59.683Z
Learning: When EHR handlers in mapping objects are set to `undefined` and there are downstream PRs mentioned in the PR objectives, this typically indicates a staged rollout approach where the core structure is implemented first and specific handler implementations follow in subsequent PRs.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/__tests__/hl7v2-roster-generator.test.ts:190-191
Timestamp: 2025-05-27T15:49:16.573Z
Learning: The user lucasdellabella prefers to keep changes focused on the PR's scope and avoid making ancillary performance improvements (like replacing delete operators with undefined assignments) that are not directly related to the main objectives of the current PR.
Learnt from: leite08
PR: metriport/metriport#3463
File: packages/api-sdk/src/medical/models/patient.ts:1-1
Timestamp: 2025-03-19T13:58:17.253Z
Learning: When changes are made to SDK packages (`api-sdk`, `commonwell-sdk`, `carequality-sdk`) or `packages/shared` in the Metriport codebase, alpha versions need to be published to NPM before merging the PR.
Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.
Learnt from: thomasyopes
PR: metriport/metriport#3709
File: packages/infra/lib/ehr-nested-stack.ts:470-486
Timestamp: 2025-04-23T19:33:45.489Z
Learning: The `refreshEhrBundles` Lambda in the EhrNestedStack uses the API to refresh bundles, rather than accessing the S3 bucket directly. This is why it doesn't need the EHR_BUNDLE_BUCKET_NAME environment variable or direct S3 bucket permissions, unlike other similar Lambdas.
Learnt from: thomasyopes
PR: metriport/metriport#4088
File: packages/infra/lib/api-stack/fhir-converter-service.ts:169-183
Timestamp: 2025-06-24T23:24:13.227Z
Learning: For Lambda functions in the FHIR converter service infrastructure, staging and production environments should have matching configuration values (memory size, timeout, etc.) rather than different values per environment.
Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:93-97
Timestamp: 2025-07-19T14:04:34.714Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes accepts SQL injection risk in the append_job_tables function's DELETE query where patient_id is directly interpolated, based on their assessment that they control all inputs to the system.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:38.884Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.
Learnt from: leite08
PR: metriport/metriport#3709
File: packages/core/src/external/ehr/bundle/create-resource-diff-bundles/steps/start/ehr-start-resource-diff-bundles-local.ts:18-24
Timestamp: 2025-04-23T21:39:58.678Z
Learning: In AWS Lambda functions, instances created inside the handler function (not as global variables) are recreated on each invocation, ensuring no state leakage between invocations, as seen with `EhrStartResourceDiffBundlesLocal` in the ehr-start-resource-diff-bundles Lambda.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/Dockerfile.lambda:1-1
Timestamp: 2025-07-19T14:03:24.392Z
Learning: In packages/data-transformation/fhir-to-csv/Dockerfile.lambda, thomasyopes prefers to use AWS Lambda base image tags (like python:3.12) as documented by AWS rather than pinning to specific digests. The team follows AWS official documentation for Lambda base images.
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/lambdas/src/ehr-compute-resource-diff.ts:29-30
Timestamp: 2025-04-19T13:19:25.570Z
Learning: Avoid logging raw SQS message bodies in EHR-related lambdas as they may contain PHI (Protected Health Information) like lists of healthcare resources that shouldn't be exposed in CloudWatch logs.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:0-0
Timestamp: 2025-07-18T09:54:43.558Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes prefers to log and swallow exceptions in the copy_into_temp_table function rather than re-raising them. This allows the data transformation job to continue processing other files even if one file fails to copy to Snowflake, supporting partial success scenarios in batch data processing.
Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.
Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.
Learnt from: leite08
PR: metriport/metriport#3912
File: packages/core/src/command/consolidated/search/fhir-resource/search-consolidated.ts:74-83
Timestamp: 2025-06-04T00:14:38.041Z
Learning: User leite08 prefers incremental progress in refactoring scenarios, accepting some inconsistency as a stepping stone towards better architecture rather than requiring perfect consistency immediately.
packages/core/src/command/analytics-platform/api/start-fhir-to-csv-transform.ts (15)
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.544Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts:18-25
Timestamp: 2025-06-25T01:56:08.732Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts, the ConversionFhirDirect class is designed for local testing purposes and does not require comprehensive error handling for HTTP requests, as confirmed by the user thomasyopes.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: CR
PR: metriport/metriport#0
File: .cursorrules:0-0
Timestamp: 2025-07-18T17:26:58.001Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : top-level comments go after the import (save pre-import to basic file header, like license)
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/ehr/elation/chart.ts:20-37
Timestamp: 2025-03-17T15:30:34.647Z
Learning: The request body parsing in packages/api/src/routes/ehr/elation/chart.ts is tracked with TODO referencing issue #2170 and will be handled in a separate PR, not in PR #3466.
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.
Learnt from: lucasdellabella
PR: metriport/metriport#3532
File: packages/utils/src/fhir-server/remove-docrefs.ts:27-27
Timestamp: 2025-05-13T16:18:27.014Z
Learning: For scripts in the codebase, configuration variables that require user modification before execution (like IDs, API endpoints, etc.) should be clearly documented with explicit TODO comments and instructions to help onboarding of new team members.
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Learnt from: leite08
PR: metriport/metriport#4073
File: packages/shared/src/common/env-var.ts:20-27
Timestamp: 2025-06-23T16:50:06.930Z
Learning: In TypeScript files, avoid creating arrow functions and use regular function declarations instead. For example, instead of `export const myFunction = (param: string): string => { ... }`, use `export function myFunction(param: string): string { ... }`. This applies to all exported functions in the codebase.
Learnt from: thomasyopes
PR: metriport/metriport#4061
File: packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts:34-47
Timestamp: 2025-06-19T22:44:49.393Z
Learning: In packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team prefers to keep refreshEhrBundles operations grouped using fire-and-forget pattern rather than awaiting all operations with Promise.allSettled(). This allows individual refresh operations to run independently without blocking the job runner.
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:27-49
Timestamp: 2025-05-28T19:22:09.281Z
Learning: In packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:0-0
Timestamp: 2025-05-28T19:20:47.442Z
Learning: In packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
Learnt from: thomasyopes
PR: metriport/metriport#3788
File: packages/api/src/external/ehr/shared/utils/bundle.ts:83-93
Timestamp: 2025-05-08T19:41:36.533Z
Learning: In the Metriport codebase, the team prefers to let errors bubble up naturally in some cases rather than adding explicit error handling at every layer, as demonstrated in the refreshEhrBundle function in the bundle.ts file.
packages/infra/lib/api-stack.ts (8)
Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/infra/lib/api-stack.ts:538-542
Timestamp: 2025-04-23T19:04:20.182Z
Learning: The `createAPIService` function in `packages/infra/lib/api-stack/api-service.ts` uses inline type definition in its parameter and does not use an `APIServiceProps` interface.
Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T19:59:22.396Z
Learning: The file at `packages/api/src/aws/app-config.ts` contains domain feature flags (not AWS AppConfig specific functionality) and is planned to be moved to the domain folder in packages/core in a future PR.
Learnt from: leite08
PR: metriport/metriport#3545
File: packages/infra/lib/api-stack.ts:383-391
Timestamp: 2025-03-28T23:03:04.409Z
Learning: When creating a nested stack in AWS CDK, don't include "NestedStack" in the ID parameter since the class name already has that suffix.
Learnt from: leite08
PR: metriport/metriport#3759
File: packages/api/src/command/medical/document/document-download.ts:14-14
Timestamp: 2025-04-30T21:03:17.716Z
Learning: The `makeS3Client()` function in `packages/api/src/external/aws/s3.ts` already initializes the AWS region internally by calling `Config.getAWSRegion()` and doesn't require a region parameter to be passed explicitly.
Learnt from: leite08
PR: metriport/metriport#3817
File: .github/workflows/deploy-staging.yml:206-214
Timestamp: 2025-05-12T18:29:48.116Z
Learning: In the Metriport CI/CD workflows, the IHE stack deployment is intentionally triggered by general infrastructure changes (infra-lambdas) rather than having its own dedicated change detection, as it needs to stay in sync with other infrastructure components.
Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.
Learnt from: thomasyopes
PR: metriport/metriport#3466
File: packages/api/src/routes/internal/internal.ts:0-0
Timestamp: 2025-03-17T23:45:45.512Z
Learning: The internal.ts file in packages/api/src/routes/internal/ is excluded from PR changes related to replacing console.log with out().log.
packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts (17)
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts:18-25
Timestamp: 2025-06-25T01:56:08.732Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-direct.ts, the ConversionFhirDirect class is designed for local testing purposes and does not require comprehensive error handling for HTTP requests, as confirmed by the user thomasyopes.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.544Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.
Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.
Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/sftp/surescripts/client.ts:1-134
Timestamp: 2025-05-28T19:23:20.179Z
Learning: In packages/core/src/external/sftp/surescripts/client.ts, the standalone getPatientLoadFileName function intentionally omits the transmission ID from the filename, which differs from the class method getPatientLoadFileName. This difference in filename generation is expected behavior.
Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.
Learnt from: leite08
PR: metriport/metriport#4073
File: packages/shared/src/common/env-var.ts:20-27
Timestamp: 2025-06-23T16:50:06.930Z
Learning: In TypeScript files, avoid creating arrow functions and use regular function declarations instead. For example, instead of `export const myFunction = (param: string): string => { ... }`, use `export function myFunction(param: string): string { ... }`. This applies to all exported functions in the codebase.
Learnt from: leite08
PR: metriport/metriport#3944
File: packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts:53-53
Timestamp: 2025-06-01T02:28:19.913Z
Learning: In packages/core/src/command/consolidated/search/fhir-resource/ingest-lexical.ts, the processErrors function intentionally throws MetriportError to bubble errors up the call stack rather than handling them locally. This is by design - errors from ingestPatientConsolidated should propagate upward rather than being caught at immediate calling locations.
Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.
Learnt from: leite08
PR: metriport/metriport#4193
File: packages/carequality-sdk/src/client/carequality-fhir.ts:198-208
Timestamp: 2025-07-16T00:54:56.156Z
Learning: User leite08 accepted the risk of parameter mutation in packages/carequality-sdk/src/client/carequality-fhir.ts when modifying org.active = false directly on input parameters, choosing to proceed despite the violation of the "Avoid modifying objects received as parameter" coding guideline.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:38.884Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.
Learnt from: thomasyopes
PR: metriport/metriport#4061
File: packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts:34-47
Timestamp: 2025-06-19T22:44:49.393Z
Learning: In packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team prefers to keep refreshEhrBundles operations grouped using fire-and-forget pattern rather than awaiting all operations with Promise.allSettled(). This allows individual refresh operations to run independently without blocking the job runner.
Learnt from: thomasyopes
PR: metriport/metriport#3383
File: packages/api/src/routes/ehr-internal/canvas/internal/patient.ts:10-24
Timestamp: 2025-03-05T21:42:32.929Z
Learning: The processPatientsFromAppointments endpoint in the Canvas API is intentionally designed to be asynchronous (fire-and-forget pattern). The endpoint returns a 200 OK response immediately after initiating the process, without waiting for it to complete. Error handling is handled through the .catch() method with processAsyncError rather than awaiting the promise.
Learnt from: leite08
PR: metriport/metriport#4034
File: packages/core/src/command/patient-import/patient-or-record-failed.ts:19-30
Timestamp: 2025-06-16T17:03:23.069Z
Learning: In patient import error handling functions like setPatientOrRecordFailed, the team prefers Promise.all over Promise.allSettled to ensure atomic all-or-nothing behavior - if either the job update or patient record update fails, the entire operation should fail rather than allowing partial success.
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:27-49
Timestamp: 2025-05-28T19:22:09.281Z
Learning: In packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.
Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:0-0
Timestamp: 2025-05-28T19:20:47.442Z
Learning: In packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.
packages/lambdas/src/analytics-platform/fhir-to-csv.ts (13)
Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.
Learnt from: leite08
PR: metriport/metriport#3709
File: packages/core/src/external/ehr/bundle/create-resource-diff-bundles/steps/start/ehr-start-resource-diff-bundles-local.ts:18-24
Timestamp: 2025-04-23T21:39:58.678Z
Learning: In AWS Lambda functions, instances created inside the handler function (not as global variables) are recreated on each invocation, ensuring no state leakage between invocations, as seen with `EhrStartResourceDiffBundlesLocal` in the ehr-start-resource-diff-bundles Lambda.
Learnt from: thomasyopes
PR: metriport/metriport#3709
File: packages/infra/lib/ehr-nested-stack.ts:470-486
Timestamp: 2025-04-23T19:33:45.489Z
Learning: The `refreshEhrBundles` Lambda in the EhrNestedStack uses the API to refresh bundles, rather than accessing the S3 bucket directly. This is why it doesn't need the EHR_BUNDLE_BUCKET_NAME environment variable or direct S3 bucket permissions, unlike other similar Lambdas.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.544Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:38.884Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.
Learnt from: leite08
PR: metriport/metriport#3857
File: packages/lambdas/src/consolidated-search.ts:32-36
Timestamp: 2025-05-27T23:51:04.692Z
Learning: In Lambda functions, prefer to let errors bubble up rather than adding explicit try-catch blocks around main operations, as the Lambda runtime and capture.wrapHandler already provide error handling and logging.
Learnt from: thomasyopes
PR: metriport/metriport#3427
File: packages/lambdas/src/ehr-sync-patient.ts:31-33
Timestamp: 2025-03-11T19:12:22.472Z
Learning: In this codebase, a specific logging pattern is used: `console.log` is used for early logging before context is available, then a prefixed logger (created with `prefixedLog`) is initialized after parsing data to include contextual information (like ehr, cxId, practiceId, patientId) in all subsequent log messages.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:0-0
Timestamp: 2025-07-18T09:54:43.558Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes prefers to log and swallow exceptions in the copy_into_temp_table function rather than re-raising them. This allows the data transformation job to continue processing other files even if one file fails to copy to Snowflake, supporting partial success scenarios in batch data processing.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:93-97
Timestamp: 2025-07-19T14:04:34.714Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes accepts SQL injection risk in the append_job_tables function's DELETE query where patient_id is directly interpolated, based on their assessment that they control all inputs to the system.
Learnt from: lucasdellabella
PR: metriport/metriport#3855
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts:88-104
Timestamp: 2025-05-16T02:10:01.792Z
Learning: When logging file paths or other strings that might contain patient identifiers like patientId or cxId, these should be masked or redacted (e.g., using regex replacement) to avoid logging Protected Health Information (PHI) to CloudWatch or error tracking systems like Sentry, ensuring HIPAA compliance.
Learnt from: thomasyopes
PR: metriport/metriport#4061
File: packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts:34-47
Timestamp: 2025-06-19T22:44:49.393Z
Learning: In packages/api/src/external/ehr/shared/job/bundle/create-resource-diff-bundles/run-job.ts, the team prefers to keep refreshEhrBundles operations grouped using fire-and-forget pattern rather than awaiting all operations with Promise.allSettled(). This allows individual refresh operations to run independently without blocking the job runner.
Learnt from: leite08
PR: metriport/metriport#4034
File: packages/core/src/command/patient-import/patient-or-record-failed.ts:19-30
Timestamp: 2025-06-16T17:03:23.069Z
Learning: In patient import error handling functions like setPatientOrRecordFailed, the team prefers Promise.all over Promise.allSettled to ensure atomic all-or-nothing behavior - if either the job update or patient record update fails, the entire operation should fail rather than allowing partial success.
packages/infra/lib/analytics-platform/analytics-platform-stack.ts (11)
Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.
Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/infra/lib/api-stack.ts:538-542
Timestamp: 2025-04-23T19:04:20.182Z
Learning: The `createAPIService` function in `packages/infra/lib/api-stack/api-service.ts` uses inline type definition in its parameter and does not use an `APIServiceProps` interface.
Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T19:59:22.396Z
Learning: The file at `packages/api/src/aws/app-config.ts` contains domain feature flags (not AWS AppConfig specific functionality) and is planned to be moved to the domain folder in packages/core in a future PR.
Learnt from: leite08
PR: metriport/metriport#3817
File: .github/workflows/deploy-staging.yml:206-214
Timestamp: 2025-05-12T18:29:48.116Z
Learning: In the Metriport CI/CD workflows, the IHE stack deployment is intentionally triggered by general infrastructure changes (infra-lambdas) rather than having its own dedicated change detection, as it needs to stay in sync with other infrastructure components.
Learnt from: thomasyopes
PR: metriport/metriport#3545
File: packages/lambdas/src/write-to-s3.ts:37-37
Timestamp: 2025-03-28T23:56:32.920Z
Learning: In the Metriport codebase, Lambdas that process messages from queues (like write-to-s3.ts) should directly use the local implementation (e.g., ProcessWriteToS3Local) rather than using the factory function. This is because the factory would queue the operation again in cloud environments, potentially creating an infinite loop. The pattern is: application code uses factory → factory queues in cloud → Lambda processes queue using local implementation.
Learnt from: leite08
PR: metriport/metriport#3545
File: packages/infra/lib/api-stack.ts:383-391
Timestamp: 2025-03-28T23:03:04.409Z
Learning: When creating a nested stack in AWS CDK, don't include "NestedStack" in the ID parameter since the class name already has that suffix.
Learnt from: thomasyopes
PR: metriport/metriport#4088
File: packages/infra/lib/api-stack/fhir-converter-service.ts:169-183
Timestamp: 2025-06-24T23:24:13.227Z
Learning: For Lambda functions in the FHIR converter service infrastructure, staging and production environments should have matching configuration values (memory size, timeout, etc.) rather than different values per environment.
Learnt from: thomasyopes
PR: metriport/metriport#3771
File: packages/lambdas/src/healthie-link-patient.ts:18-19
Timestamp: 2025-05-01T16:11:56.826Z
Learning: Environment variables that are explicitly set by the team in a controlled manner (like WAIT_TIME_IN_MILLIS in Lambda configurations) don't require additional validation checks as their values are guaranteed to be of the correct type.
Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/Dockerfile.lambda:1-1
Timestamp: 2025-07-19T14:03:24.392Z
Learning: In packages/data-transformation/fhir-to-csv/Dockerfile.lambda, thomasyopes prefers to use AWS Lambda base image tags (like python:3.12) as documented by AWS rather than pinning to specific digests. The team follows AWS official documentation for Lambda base images.
Learnt from: thomasyopes
PR: metriport/metriport#3709
File: packages/infra/lib/ehr-nested-stack.ts:470-486
Timestamp: 2025-04-23T19:33:45.489Z
Learning: The `refreshEhrBundles` Lambda in the EhrNestedStack uses the API to refresh bundles, rather than accessing the S3 bucket directly. This is why it doesn't need the EHR_BUNDLE_BUCKET_NAME environment variable or direct S3 bucket permissions, unlike other similar Lambdas.
Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.
🧬 Code Graph Analysis (1)
packages/infra/lib/analytics-platform/analytics-platform-stack.ts (1)
packages/infra/lib/shared/secrets.ts (1)
Secrets(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
packages/core/src/command/analytics-platform/api/start-fhir-to-csv-transform.ts (1)
13-13: Clear deprecation marker for API-based approach.The TODO comment clearly indicates the shift away from API-based transform initiation to direct function calls, which aligns with the overall refactoring strategy in this PR.
packages/infra/lib/api-stack.ts (2)
3-3: Import reordering is acceptable.The reordering of the
aws_wafv2import maintains alphabetical order and doesn't affect functionality.
516-516: Secrets integration enables secure credential management.Adding the
secretsparameter to the AnalyticsPlatformsNestedStack properly enables the secure Snowflake credentials handling for the FHIR-to-CSV Lambda workflow.packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts (2)
2-2: Clean refactoring to direct function invocation.The import change from API-based to direct function call is well-executed and maintains the same interface while removing network dependencies.
14-14: Function call updated consistently with import change.The function invocation correctly uses the new direct transform function with the same parameters, maintaining functionality while improving efficiency.
packages/lambdas/src/analytics-platform/fhir-to-csv.ts (2)
1-1: Proper AWS Secrets Manager integration.The import of
getSecretfrom AWS Lambda Powertools is the correct approach for secure secret retrieval in Lambda functions.
16-16: Environment variable for secret name is correctly configured.Using
getEnvOrFailensures the secret name is properly configured and fails fast if missing.packages/infra/lib/analytics-platform/analytics-platform-stack.ts (6)
17-17: LGTM: Import additions are correctThe new imports for secret management functionality are properly added and align with the infrastructure changes.
Also applies to: 19-19
89-89: LGTM: Props interface correctly updatedAdding the required
secretsproperty to the props interface is the correct approach for dependency injection of secrets into the nested stack.
180-180: LGTM: Secrets parameter threading is correctThe secrets are properly passed from the constructor props to the
setupFhirToCsvLambdamethod, following the established parameter passing pattern.Also applies to: 226-226
304-305: LGTM: Environment variables correctly updated for secure secret handlingThe changes properly implement secure secret management by:
- Passing the Lambda function name for direct invocation
- Providing the secret name for runtime retrieval instead of embedding secrets in environment variables
314-315: LGTM: IAM permissions correctly configuredThe permission grants properly enable:
- Direct Lambda-to-Lambda invocation
- Secret read access for runtime credential retrieval
This follows the principle of least privilege.
319-319: LGTM: Helpful TODO comment for future refactoringThe TODO comment clearly indicates the planned future removal of the transform lambda and associated infrastructure, providing valuable context for the current transitional state.
Ref eng-602 Signed-off-by: Rafael Leite <2132564+leite08@users.noreply.github.com>
thomasyopes
approved these changes
Jul 20, 2025
This was referenced Jul 25, 2025
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dependencies
Description
FhirToCsv lambda calls the transform one directly - context
Testing
see upstream
Release Plan
Summary by CodeRabbit
New Features
Chores
Refactor