Implemented API: 4.8 Make response of GET /collections and GET /collections/{id} STAC-Conform - #201
Implemented API: 4.8 Make response of GET /collections and GET /collections/{id} STAC-Conform#201georgevoulg wants to merge 4 commits into
Conversation
…ctions/{id} STAC-Conform
STAC API Validator - Conformance Validation Complete
Successfully ran the STAC API Validator locally against the API and confirmed full conformance with both Core and Collections specifications.
1. stac_extensions field - Changed from null to empty array [] (STAC spec requires array type)
Updated: routes/collections.js
2.Collection type field - Added type: 'Collection' to all in-memory test collections
Updated: data/collections.js
3. String IDs - Converted numeric database IDs to strings (STAC requires string IDs)
Updated: routes/collections.js
4. Provider roles as arrays - Changed from comma-separated string to proper array
Updated: buildCollectionSearchQuery.js using string_to_array()
5. Remove null optional fields - Assets and summaries now omitted if null (not included as null)
Updated: routes/collections.js
6. Test updated - Fixed test expectation for provider roles SQL pattern
Updated: buildCollectionSearchQuery.aggregates.test.js
RobinGummels
left a comment
There was a problem hiding this comment.
Please remove absolutely EVERYTHING which belongs to your in-memory db. I don't know, why it should be helpfull to work with this, but as you are using it as a fallback if something is wrong with the real db to skip tests it is harshly against the CI-principles.
Furthermore some of you code is kinda nonsense to me. I don't get, why you made certain changes. Further explanation (in code and chat) would be very helpfull.
| const sortField = sortby.field; | ||
| const dir = sortby.direction; | ||
| // Apply ASCII collation only for license to match deterministic tests | ||
| const collate = sortField === 'license' ? ' COLLATE "C"' : ''; | ||
| sql += ` ORDER BY c.${sortField}${collate} ${dir}, c.id ASC`; |
There was a problem hiding this comment.
Explain me, why you are doing this
There was a problem hiding this comment.
The code sorts collections by a specific field (e.g. license or title). When sorting by license, an ASCII-based sort order is intentionally used. The reason is that different machines could otherwise produce different sort orders, depending on locale and database settings. By using COLLATE "C", the sorting is identical on all systems, ensuring that tests remain reproducible everywhere. If you think we don’t need it I can remove it.
There was a problem hiding this comment.
Please remove every of you changes which belongs to the in-memory fallback. I don't see a reason, why we should need this.
There was a problem hiding this comment.
I saw somewhere that it’s optional for devs and that the summaries in this file are recommended (maybe I’m mixing something up), but I’ll delete it.
https://github.com/radiantearth/stac-spec/blob/master/collection-spec/collection-spec.md#collection-fields
There was a problem hiding this comment.
Ask @SonkeHoffmann for this. He's the expert for PostGIS. But can you explain me, what this script should do? I don't realy understand.
| it('should return STAC Collection object with required fields', async () => { | ||
| const response = await request(app).get('/collections/sentinel-2-l2a').expect(200); | ||
|
|
||
| expect(response.body).toHaveProperty('id', 'sentinel-2-l2a'); | ||
| expect(response.body).toHaveProperty('stac_version'); | ||
| expect(response.body).toHaveProperty('title'); | ||
| expect(response.body).toHaveProperty('description'); | ||
| expect(response.body).toHaveProperty('license'); | ||
| expect(response.body).toHaveProperty('extent'); | ||
| expect(response.body).toHaveProperty('links'); | ||
| expect(Array.isArray(response.body.links)).toBe(true); | ||
| }); | ||
|
|
||
| it('should include self and root links', async () => { | ||
| const response = await request(app).get('/collections/sentinel-2-l2a').expect(200); | ||
|
|
||
| const links = response.body.links; | ||
| const linkRels = links.map(link => link.rel); | ||
|
|
||
| expect(linkRels).toContain('self'); | ||
| expect(linkRels).toContain('root'); |
There was a problem hiding this comment.
If i'm correct this also only response to your in-memory db. It makes no sense using this.
There was a problem hiding this comment.
Changes :
Dynamically fetch the first available collection from /collections
Use that collection for the tests
Or skip the tests if no collections are available
| expect(sql).toMatch(/jsonb_agg\(jsonb_build_object\(/); | ||
| expect(sql).toMatch(/'name', p\.provider/); | ||
| expect(sql).toMatch(/'roles', cpr\.collection_provider_roles/); | ||
| expect(sql).toMatch(/'roles', string_to_array\(cpr\.collection_provider_roles, ','\)/); |
There was a problem hiding this comment.
Is there a benefit, if we turn the providers into an array?
There was a problem hiding this comment.
STAC compliance: providers[*].roles is defined by the STAC specification as an array of strings. A comma-separated string would be incorrectly typed.
Validator: The STAC API Validator expects an array; otherwise the schema validation fails.
Clients: Frontends/clients can directly use roles.includes('producer'), etc., instead of having to implement string splitting.
Data quality: This prevents typos and errors during later splitting and keeps the typing consistent.
|
I already commented you Screenshots in the Issue... #192 |
- Deleted data folder - api.test.js tests first collection from the db - db_APIconnection changed some IS TEST
SonkeHoffmann
left a comment
There was a problem hiding this comment.
To be honest, I don't think we need this file, please explain. I'm open for new Ideas and change ideas when I am convinced of the changes.
There was a problem hiding this comment.
It looks like a typical AI file. An AI like Chat likes to use seed date to get an entry point into the database or to initialise the database. But since the databases (5432 & 5433) are already running I don't see the point in using this file. There are already data in the databases, so it doesn't make any sense to add a starting/initialising data. If I missed a point and it is actually helpful to use such a file, please explain.
Another point: We don't write INSERT statements inside db/init. These files are only for stating an empty database fron the docker container. If you want to have at the file, then make that in another folder that won't be started by the docker-compose.yml, you can add the data per hand later.
| WHERE id = v_collection_id AND temporal_extend_end IS NULL; | ||
|
|
||
| INSERT INTO collection_keywords (collection_id, keyword_id) | ||
| SELECT v_collection_id, k.id FROM keywords k |
There was a problem hiding this comment.
Are you sure that k.id works? I know that you used it similar in the API component, but I'm not sure cause the columns are only named id etc.
| END $$; | ||
|
|
||
| -- Landsat 8 Level-1 | ||
| DO $$ |
There was a problem hiding this comment.
are those real data that you got from stac, or are you just using the data suggested by AI. Those dummy data can be helpful in the production, but I don't wanna have data in the pre-Release (and in the actual Release) that are just dummy and not real.
| @@ -0,0 +1,190 @@ | |||
| -- I did not wrote this code myself. It was generated by ChatGPT based on my instructions. It needs to be reviewed and tested. | |||
There was a problem hiding this comment.
documentation is very low for such a long file. Maybe add where you got the data from and why we need this file.
| -- This script can be executed repeatedly without creating duplicates. | ||
|
|
||
| -- Root catalog (optional) | ||
| INSERT INTO catalog (stac_version, type, title, description) |
There was a problem hiding this comment.
Why are you only adding these data into the catalogs an not every data referred inside the catalog. It looks like you did that for collections, but not for catalogs.
Am I missing something?
and deleted everything with the in memory data
|
Insufficient code quality. Missed the goal |
Implemented API: 4.8 Make response of GET /collections and GET /collections/{id} STAC-Conform
STAC API Validator - Conformance Validation Complete
Successfully ran the STAC API Validator locally against the API and confirmed full conformance with both Core and Collections specifications.
Updated: routes/collections.js
2.Collection type field - Added type: 'Collection' to all in-memory test collections
Updated: data/collections.js
String IDs - Converted numeric database IDs to strings (STAC requires string IDs)
Updated: routes/collections.js
Provider roles as arrays - Changed from comma-separated string to proper array
Updated: buildCollectionSearchQuery.js using string_to_array()
Remove null optional fields - Assets and summaries now omitted if null (not included as null)
Updated: routes/collections.js
Test updated - Fixed test expectation for provider roles SQL pattern
Updated: buildCollectionSearchQuery.aggregates.test.js
I created a file (06_seed_data.sql)in the DB folder with the help of ChatGPT. Everything is working correctly, but it still needs to be reviewed by someone
All test passed

