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

Fix database entity detached issues on mission update #1085

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

aeshub
Copy link
Contributor

@aeshub aeshub commented Oct 27, 2023

No description provided.

@aeshub aeshub added the bug Something isn't working label Oct 27, 2023
@aeshub aeshub self-assigned this Oct 27, 2023
@aeshub aeshub changed the title Fix database errors Fix database entity detached issues on mission update Oct 27, 2023
@github-actions
Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@aeshub aeshub force-pushed the fix-database-errors branch from bfca331 to 3685da6 Compare October 27, 2023 08:17
Copy link
Contributor

@andchiind andchiind left a comment

Choose a reason for hiding this comment

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

This might be quite hard to do but would it maybe be possible to split away the change which fixes the database entity detached issue into a separate commit? Since currently it’s a bit hard to tell what changes are necessary in order to make sure the right context objects are used, and which changes were made to improve the code readability. No stress if the refactoring and fix are tightly coupled though.

backend/api/EventHandlers/MqttEventHandler.cs Show resolved Hide resolved
backend/api/EventHandlers/MqttEventHandler.cs Show resolved Hide resolved
backend/api/EventHandlers/MqttEventHandler.cs Show resolved Hide resolved
backend/api/EventHandlers/MqttEventHandler.cs Show resolved Hide resolved
backend/api/EventHandlers/MqttEventHandler.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Eddasol Eddasol left a comment

Choose a reason for hiding this comment

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

LGTM :)

@aeshub aeshub force-pushed the fix-database-errors branch from 3685da6 to f831bb6 Compare October 27, 2023 10:41
@aeshub
Copy link
Contributor Author

aeshub commented Oct 27, 2023

This might be quite hard to do but would it maybe be possible to split away the change which fixes the database entity detached issue into a separate commit? Since currently it’s a bit hard to tell what changes are necessary in order to make sure the right context objects are used, and which changes were made to improve the code readability. No stress if the refactoring and fix are tightly coupled though.

The refactoring done in the second commit is basically the fix. So we wrap the different scoped services into a separate one so that we are not calling multiple different scoped services within the same event handler. So in a sense the second commit although it looks mostly like refactoring is the fix.

This moves the database calls into a separate action service which
means we resolve the database detached entity errors encountered in the
OnMissionUpdate event handler.
@aeshub aeshub force-pushed the fix-database-errors branch from 412968e to d08e07c Compare October 27, 2023 10:47
@aeshub aeshub force-pushed the fix-database-errors branch from d08e07c to dd402c5 Compare October 27, 2023 10:51
@aeshub aeshub merged commit 653820b into equinor:main Oct 27, 2023
8 checks passed
@aeshub aeshub deleted the fix-database-errors branch October 27, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants