Alternative Collection Concept Id in the signed s3 urls #830
Conversation
WalkthroughAdds a collection ID cache and retrieval flow: new LRU Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServiceResults as ServiceResults(frontend)
participant Cache as collectionIdCache
participant JobModel as Job.getCollectionIdForJobId
participant Signer as RequestSigner
Client->>ServiceResults: request service result (jobId)
alt jobId present
ServiceResults->>Cache: fetch(jobId)
alt cache hit
Cache-->>ServiceResults: collectionIds
else cache miss
Cache-->>ServiceResults: (miss)
ServiceResults->>JobModel: getCollectionIdForJobId(jobId)
JobModel-->>ServiceResults: collectionIds (or undefined)
ServiceResults->>Cache: store(jobId, collectionIds)
end
ServiceResults->>Signer: include header A-provider, A-collection-concept-ids (if collectionIds)
else no jobId
ServiceResults->>Signer: include header A-provider only
end
Signer-->>ServiceResults: signed request
ServiceResults-->>Client: redirect / service result response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flamingbear
requested review from
chris-durbin,
indiejames and
ygliuvt
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
services/harmony/test/service-results.ts (1)
68-84: Consider adding edge case test coverage.The test correctly validates the happy path where collection IDs exist and are formatted as expected. However, consider adding a test case for when a job has no associated collection IDs (where
collectionIdCache.fetchreturnsundefined). This would ensure the header is omitted gracefully rather than causing errors.Example test case
describe('when the job has no collection IDs', function () { let providerIdCacheStub, collectionIdCacheStub; before(function () { providerIdCacheStub = sinon.stub(providerIdCache, 'fetch').resolves('eedtest'); collectionIdCacheStub = sinon.stub(collectionIdCache, 'fetch').resolves(undefined); }); after(function () { providerIdCacheStub.restore(); collectionIdCacheStub.restore(); }); hookUrl('/service-results/some-bucket/public/some-job-id/some-work-item-id/some-path.tif', 'jdoe'); it('does not include the A-collection-concept-ids header', function () { expect(this.res.headers.location).to.not.include('A-collection-concept-ids'); }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/harmony/app/frontends/service-results.ts(3 hunks)services/harmony/app/models/job.ts(1 hunks)services/harmony/test/service-results.ts(2 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
services/harmony/app/frontends/service-results.ts (1)
138-140: Add defensive type check to prevent runtime error.The code calls
.toUpperCase()oncollectionIdswithout verifying it's a string. Based on theJob.getCollectionIdForJobIdimplementation (see relevant snippet), the method can returnundefined(when no results are found) or potentially an array, despite itsPromise<string>signature. Calling.toUpperCase()on non-string values will cause a runtime error.🔎 Proposed fix
if (collectionIds) { - customParams['A-collection-concept-ids'] = collectionIds.toUpperCase(); + customParams['A-collection-concept-ids'] = String(collectionIds).toUpperCase(); }Alternatively, add a type guard:
- if (collectionIds) { + if (collectionIds && typeof collectionIds === 'string') { customParams['A-collection-concept-ids'] = collectionIds.toUpperCase(); }Note: The root cause should also be addressed in
Job.getCollectionIdForJobIdto ensure it always returns a string or explicitly returnsstring | undefined, and handles the undefined case properly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
services/cron-service/test/resources/test-env-defaults(1 hunks)services/harmony/app/frontends/service-results.ts(3 hunks)services/harmony/app/util/env.ts(1 hunks)services/harmony/env-defaults(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/harmony/app/frontends/service-results.ts (1)
services/harmony/app/models/job.ts (1)
Job(431-1248)
⏰ 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). (3)
- GitHub Check: compare-services
- GitHub Check: build (22.14.x)
- GitHub Check: build (22.14.x)
🔇 Additional comments (3)
services/harmony/app/util/env.ts (1)
132-138: LGTM!The collection cache configuration fields are properly validated with appropriate constraints and follow the established pattern for other cache settings.
services/harmony/app/frontends/service-results.ts (2)
79-94: LGTM!The
fetchCollectionIdfunction is well-structured and follows the established pattern offetchProviderId. The JSDoc correctly describes the function's behavior.
96-102: LGTM!The
collectionIdCacheconfiguration properly mirrors theproviderIdCachepattern with appropriate TTL, size limits, and fetch method.
I don't think this is strictly necessary, but I also don't think it was right before.
flamingbear
changed the title
|
Closing this since I was able to get #827 working properly |
Jira Issue ID
Originally I couldn't fix my other PR. But I did manage to. I think this is the alternative now in the off chance you want both caches separate.
None general improvements for NSIDC (and others) metrics
NOTE: I was trying to combine the DB lookup for providerID with the collectionIds, in #827, but I am just failing with my cache so far. I am over my head on that one. I don't know if the extra function call in this version, where I just follow the existing providerId pattern causes enough overhead to keep trying to figure out what I'm doing wrong on the other PR, or if this is an acceptable solution.Description
Adds an additional query parameter of
A-collection-concept-idswhich is a comma separated list of collection concept ids associated with the JOBID. In all cases now, this is a single value.Local Test Steps
Pull this branch. Run the tests. Build local images and run Harmony-In-A-Box.
Run a sample request. e.g.:
http://localhost:3000/C1238392622-EEDTEST/ogc-api-coverages/1.0.0/collections/parameter_vars/coverage/rangeset?forceAsync=true&subset=lat(0%3A15)&subset=lon(-150%3A-105)&label=harmony-py&granuleId=G1245840464-EEDTEST&variable=atmosphere_cloud_liquid_water_content
Look at the logs in the
k8s_harmony_harmonypod when you download the result and look for the logs that show the signing parameters. see that a new value"A-collection-concept-ids":"C1238392622-EEDTEST"is includedPR Acceptance Checklist
Documentation updated (if needed)Summary by CodeRabbit
New Features
Tests
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.