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 bad queries after adding new propreties to entity list #6402

Merged
merged 9 commits into from
Sep 10, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Sep 9, 2024

Closes #6396

Why is this the best possible solution? Were any other approaches considered?

As per the discussion in #6396, the conclusion was eventually made that the root cause was due to ALTER TABLE statements being executed in a shared database connection (SqlLiteOpenHelper). The solution here is to reset that connection (using a new DatabaseConnetion#reset method) so that follow-up queries do not end up getting incorrect/state data or data structures.

I've also added a new SynchronizedDatabaseConnection class that allows us to interact with DatabaseConnection in a synchornized way to stop prevent any problems that might come up from concurrent access combined with reset(). The reason for this is described in a comment on reset():

/**
 * Closes the underlying connection and clears state so that subsequent accesses will create
 * a new connection.
 *
 * This can be dangerous if the database is being access by multiple threads as the connection
 * might be closed while a transaction is running or while another thread is using a
 * [SQLiteDatabase] instance obtained via [writeableDatabase] or [readableDatabase]. Using
 * [SynchronizedDatabaseConnection] is recommended in those scenarios.
 */

Using synchornized access for DatabaseConnection in DatabaseEntitiesRepository will theoretically have performance implications as two threads will not be able to simultaneously read from the DB. To some level this was already not possible (as SqlLiteOpenHelper serializes calls to the actual underlying database), but it will certainly expand the amount of "blocking" code. The real world impact for entities is basically non-existent as we're planning to remove concurrent form entry/form updates anyway in #6232 which would be the only situation that would benefit as far as I can tell. I think adding this lower level protection is important to reduce the risk of adding race conditions down the road.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This should just fix the bug. Checking entity form downloads and form entry would be good here as this is a fairly low level change that could always have unforeseen consequences.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

This should prevent issues with reset() being called while another
transaction is running or a thread has a reference to a writable/
readable database.
@dbemke
Copy link

dbemke commented Sep 9, 2024

I'm not able to reproduce the issue #6396 (error doesn't occur) and I haven't found any errors connected with adding properties in Central so it seems that it's fixed

body: SQLiteDatabase.() -> T
) {
transaction(body)
databaseConnection.reset()
Copy link
Member Author

@seadowg seadowg Sep 9, 2024

Choose a reason for hiding this comment

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

I'd been unable to write a Robolectric test that would drive out adding this reset() with the existing implementation. I'm guessing that it may have been possible to write one in an instrumentation test.

However, now if you remove reset(), a few tests fail in the existing DatabaseEntitiesRepository. This is because the change to looking up columns to determine which to add (using ALTER TABLE) is much more sensitive to the problem than the previous implementation that used a try-catch to just "fire and forget" add columns. I'm leaning towards not bothering with a new instrumentation test as it feels unlikely that anyone would go back to the old hacky implementation, but happy to debate that!

@seadowg seadowg marked this pull request as ready for review September 9, 2024 12:38
@seadowg seadowg requested a review from grzesiek2010 September 9, 2024 12:38
@seadowg seadowg added the high priority Should be looked at before other PRs/issues label Sep 9, 2024
Copy link
Member

@grzesiek2010 grzesiek2010 left a comment

Choose a reason for hiding this comment

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

There are still some occurrences of writeable in DatabaseFormsRepository please fix them as well.

@grzesiek2010 grzesiek2010 merged commit d0dfdda into getodk:master Sep 10, 2024
6 checks passed
@seadowg seadowg deleted the alter-table branch September 10, 2024 12:45
@dbemke
Copy link

dbemke commented Sep 12, 2024

Tested with Success!

Verified on a device with Android 10

Verified Cases:

@WKobus
Copy link

WKobus commented Sep 12, 2024

Tested with Success

Verified on device with Android 14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch error after finalizing forms, adding property in Central and refreshing list of blank forms
4 participants