add mosaic search + other updates #36
Conversation
| @@ -99,7 +100,7 @@ class Config: | |||
| """model config""" | |||
|
|
|||
| env_file = "deployment/.env" | |||
| env_prefix = "EOAPI_RASTER_" | |||
| env_prefix = "CDK_EOAPI_RASTER_" | |||
There was a problem hiding this comment.
This is to avoid confusion between EOAPI_RASTER_ env used in eoAPI.raster module and the ones used for deployment.
| ) as conn: | ||
| conn.execute( | ||
| sql.SQL( | ||
| "CREATE INDEX IF NOT EXISTS searches_mosaic ON searches ((true)) WHERE metadata->>'type'='mosaic';" |
There was a problem hiding this comment.
By default we create an index on search metadata->>type (to be able to list mosaic)
cc @bitner
There was a problem hiding this comment.
I think it's ok to have this here for now, but I think that it would be good to just include this index in pgstac itself and get rid of this in the future as this will add a bit of latency every time you connect.
There was a problem hiding this comment.
this is only used when we create the database!
| @@ -85,6 +85,8 @@ services: | |||
| # AWS S3 endpoint config | |||
| - AWS_ACCESS_KEY_ID=${AWS_ACCESS_KEY_ID} | |||
| - AWS_SECRET_ACCESS_KEY=${AWS_SECRET_ACCESS_KEY} | |||
| # API Config | |||
| - EOAPI_RASTER_ENABLE_MOSAIC_SEARCH=TRUE | |||
There was a problem hiding this comment.
| links=links, | ||
| numberMatched=int(nb_items), | ||
| numberReturned=len(searches_info), | ||
| ) |
There was a problem hiding this comment.
@bitner can you check that I'm not doing any stupid SQL (or python, or general) stuff.
There was a problem hiding this comment.
Nothing bad here that needs to change to merge, but some suggestions on best practices above.
| @@ -73,6 +73,7 @@ class eoRasterSettings(pydantic.BaseSettings): | |||
| "CPL_VSIL_CURL_ALLOWED_EXTENSIONS": ".tif,.TIF,.tiff", | |||
| "GDAL_CACHEMAX": "200", # 200 mb | |||
| "GDAL_DISABLE_READDIR_ON_OPEN": "EMPTY_DIR", | |||
| "GDAL_INGESTED_BYTES_AT_OPEN": "32768", | |||
There was a problem hiding this comment.
I think this is a reasonable default value
|
not at my computer, there are some other small not critical changes in your
query generation, but the big one is don't use "metadata::json" in your
filters. the metadata column is jsonB and casting it to json will cause it
to need to do the cast for every record and will prevent your index from
getting used
…On Fri, Mar 4, 2022, 5:36 AM Vincent Sarago ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/eoapi/raster/eoapi/raster/factory.py
<#36 (comment)>:
> + ),
+ ),
+ model.Link(
+ rel="tilejson",
+ href=self.url_for(
+ request, "tilejson", searchid=search.id
+ ),
+ ),
+ ],
+ )
+ for search in searches_info
+ ],
+ links=links,
+ numberMatched=int(nb_items),
+ numberReturned=len(searches_info),
+ )
@bitner <https://github.com/bitner> can you check that I'm not doing any
stupid SQL (or python, or general) stuff.
—
Reply to this email directly, view it on GitHub
<#36 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABIHXDKJ2ZFFNRSFNTLJXTU6HYUVANCNFSM5P5JKURA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
I left a couple comments about some best practices for constructing sql. None of them are critical, but things to keep in mind.
The thing I might consider changing now is to use the same object for context as is in the spec for search for consistency.
We should definitely follow up and make sure we just add that index for search metadata directly into pgstac so we can get rid of it here in the future.
| ) as conn: | ||
| conn.execute( | ||
| sql.SQL( | ||
| "CREATE INDEX IF NOT EXISTS searches_mosaic ON searches ((true)) WHERE metadata->>'type'='mosaic';" |
There was a problem hiding this comment.
I think it's ok to have this here for now, but I think that it would be good to just include this index in pgstac itself and get rid of this in the future as this will add a bit of latency every time you connect.
| ): | ||
| """List a Search query.""" | ||
| offset_and_limit = [ | ||
| sql.SQL("LIMIT {number}").format(number=sql.Literal(limit)), |
There was a problem hiding this comment.
This is OK from a SQL injection standpoint, but in general we should avoid using sql.Literal(). Literals should be put in the query as placeholders and the values passed as parameters to execute ie execute("select * from search limit %(limit)s" , {"limit":limit}). This allows psycopg to use prepared statements behind the scenes which adds a bit more security and potential performance benefits.
| links=links, | ||
| numberMatched=int(nb_items), | ||
| numberReturned=len(searches_info), | ||
| ) |
There was a problem hiding this comment.
Nothing bad here that needs to change to merge, but some suggestions on best practices above.
This PR adds a
mosaic/listendpoint to list/searchmosaicregistered in PgSTAC databaseref:
Other fix/additions
breaking changes
CDK_prefix todeploymentrelated environment variables