Skip to content
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.

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

feat(sql session): Use context manager for sql session #20

Conversation

julienloizelet
Copy link
Collaborator

Fixes #19

@julienloizelet julienloizelet self-assigned this Feb 15, 2024
@@ -143,49 +143,49 @@ class SQLStorage(storage.StorageInterface):
def __init__(self, connection_string="sqlite:///cscapi.db") -> None:
engine = create_engine(connection_string, echo=False)
Base.metadata.create_all(engine)
Session = sessionmaker(bind=engine)
self.session = Session()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we believe chatGPT:

The Session() call creates a new SQLAlchemy Session, which opens a new transaction. It's not recommended to start a transaction in the init method.

The reason is that the init method is called when an instance of the class is created. If you start a transaction in the init method, the transaction will be open for the entire lifetime of the object, which is not what you typically want. Transactions should be short-lived and should be started just before performing database operations, and ended (committed or rolled back) as soon as possible.

Instead, you should create the sessionmaker in the init method and then start a new session in each method that performs database operations, using a context manager (with statement). This ensures that each session is properly closed, even if an error occurs during the database operations.

Copy link
Collaborator

@rr404 rr404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@julienloizelet julienloizelet merged commit b2ce126 into crowdsecurity:main Feb 16, 2024
16 checks passed
julienloizelet added a commit to julienloizelet/crowdsec-python-capi-sdk that referenced this pull request Mar 14, 2024
)

* feat(sql session): Use context manager for sql session

* docs(changelog): Prepare next release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handling of sql session
2 participants