Skip to content

disable transaction and bulk_transactions extensions by default #236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
May 13, 2025
Merged

disable transaction and bulk_transactions extensions by default #236

merged 8 commits into from
May 13, 2025
+148 −41

Conversation

hrodmn
Copy link
Collaborator

@hrodmn hrodmn commented Apr 28, 2025

resolves #231

Related Issue(s):

Description:

It seems hazardous to enable the transactions extensions out-of-the-box because users might not realize that they are enabling users to POST data to the pgstac database. This changes those extensions default to off but lets users opt-in via ENABLED_EXTENSIONS environment variable.

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@hrodmn hrodmn requested a review from vincentsarago April 28, 2025 15:09
@hrodmn hrodmn assigned hrodmn and unassigned hrodmn Apr 28, 2025
@hrodmn hrodmn requested a review from gadomski April 28, 2025 15:09
@hrodmn @vincentsarago
apply suggestions from review
fbfa774 
Co-authored-by: Vincent Sarago <vincent@developmentseed.org>
@vincentsarago
Copy link
Member

Q: should we make

stac-fastapi-pgstac/stac_fastapi/pgstac/config.py

Line 68 in 476796f

postgres_host_writer: str
optional as well ?

cc @gadomski @hrodmn

@gadomski
Copy link
Member

I think so -- feels weird to have to specify it if you don't plan on supporting writes.

@vincentsarago
Copy link
Member

@gadomski @hrodmn I'm not going to change the PostgresSettings class in this PR.
I assume all user already set the postgres_host_writer variable anyway

My knowledge in postgres is limited but I'm having hard time to understand how can the writer and reader host be different cc @bitner

@vincentsarago vincentsarago mentioned this pull request May 13, 2025
Open
@vincentsarago
Copy link
Member

vincentsarago commented May 13, 2025 •
edited
Loading

Something worries me now.

If users want to enable the transaction extension using env variables they will have to set ENABLED_EXTENSIONS with ALL the extensions, not just the transactions ones.

what do we think about a new settings called ENABLE_TRANSACTIONS which will enable both transaction and bulk_transactions?

Note: this is what we do in eoapi-devseed https://github.com/developmentseed/eoapi-devseed/blob/36e0618b6573a7ce243fddd742e83deb397806cd/runtimes/eoapi/stac/eoapi/stac/app.py#L80-L95

@jonhealy1
Copy link
Collaborator

Similar: stac-utils/stac-fastapi-elasticsearch-opensearch#374

@vincentsarago
Copy link
Member

thanks @jonhealy1
I'll follow the same pattern for ENABLE_TRANSACTIONS_EXTENSIONS 🙏 (I was about to submit a PR with ENABLE_TRANSACTIONS 😅 )

@jonhealy1
Copy link
Collaborator

Haha I was thinking about changing ours to ENABLE_TRANSACTIONS :)

@vincentsarago
Copy link
Member

ENABLE_TRANSACTIONS_EXTENSIONS is 👍

@jonhealy1
Copy link
Collaborator

Ok ok ;)

vincentsarago and others added 2 commits May 13, 2025 14:05
@vincentsarago @hrodmn
use specific ENV ENABLE_TRANSACTIONS_EXTENSIONS for transactions (#239)
3749c12 
* use specific ENV ENABLE_TRANSACTIONS_EXTENSIONS for transactions

* Update docker-compose.yml

Co-authored-by: Henry Rodman <henry.rodman@gmail.com>

---------

Co-authored-by: Henry Rodman <henry.rodman@gmail.com>
@vincentsarago
update changelog
8d92252
@vincentsarago vincentsarago merged commit db9d81c into main May 13, 2025
7 checks passed
@vincentsarago vincentsarago deleted the feat/disable-transaction-extensions branch May 13, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincentsarago vincentsarago vincentsarago left review comments

@gadomski gadomski gadomski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

disable transaction and bulk transactions extensions by default?
4 participants
@hrodmn @vincentsarago @gadomski @jonhealy1