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

ErrorContextBuilder should only force embedded H2 databases to be DataStoreType.NOT_SHARED #306

Closed
sleberknight opened this issue Oct 25, 2023 · 1 comment · Fixed by #309
Assignees
Labels
enhancement A request for change or improvement to an existing feature
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Oct 25, 2023

The ErrorContextBuilder#buildWithDataStoreFactory method forces the data store type to be DataStoreType.NOT_SHARED for all H2 databases regardless of connection mode, i.e. whether using embedded mode (e.g. local file-based or in-memory) or server mode.

Only embedded databases should be forced to NOT_SHARED, since server mode H2 databases are client/server applications over TCP/IP and thus are shared.

Under the covers the buildWithDataStoreFactory method calls the public ApplicationErrorJdbc#isH2DataStore method, which only checks if the driver class is org.h2.Driver. Since this method is public, we can't just modify its behavior in a substantial manner without a major version bump. So, a better option for now is to create a new method, e.g. isEmbeddedH2DataStore, which checks both the driver class is H2, and that the (H2) JDBC URL is one for an embedded mode database, which encompasses several types of URLs. See Database URL Overview in the H2 documentation.

The private ErrorContextBuilder#forceH2DatabaseToBeNotSharedWithWarning method should be renamed, and the logged warning should change "in-memory" to be "embedded" or something more generic to indicate an embedded (and thus not shared) database.

The javadocs in ErrorContextBuilder should also be updated to state that only embedded H2 databases will be automatically set to NOT_SHARED.

@sleberknight sleberknight added the enhancement A request for change or improvement to an existing feature label Oct 25, 2023
@sleberknight sleberknight changed the title ErrorContextBuilder should only force embedded databases to be DataStoreType.NOT_SHARED ErrorContextBuilder should only force embedded H2 databases to be DataStoreType.NOT_SHARED Oct 26, 2023
@sleberknight sleberknight self-assigned this Oct 26, 2023
@sleberknight sleberknight added this to the 2.1.0 milestone Oct 26, 2023
@sleberknight
Copy link
Member Author

sleberknight added a commit that referenced this issue Oct 26, 2023
* Add isH2EmbeddedDataStore to ApplicationErrorJdbc
* Remove the erroneous implNote from isH2DataStore in
  ApplicationErrorJdbc; the method name tells you
  exactly wha it does
* Change ErrorContextBuilder to force NOT_SHARED only
  when the data store type has already been set, and the
  JDBC URL in the DataSourceFactory is an H2 URL that
  is for an embedded connection.

Closes  #306
chrisrohr pushed a commit that referenced this issue Oct 30, 2023
* Only force embedded H2 databases to be NOT_SHARED

* Add isH2EmbeddedDataStore to ApplicationErrorJdbc
* Remove the erroneous implNote from isH2DataStore in
  ApplicationErrorJdbc; the method name tells you
  exactly wha it does
* Change ErrorContextBuilder to force NOT_SHARED only
  when the data store type has already been set, and the
  JDBC URL in the DataSourceFactory is an H2 URL that
  is for an embedded connection.

Closes  #306

* Cleanup the H2 data store logic

* Update isH2DataStore to check driver and URL
* No need to check blank URL in isH2EmbeddedDataStore anymore since
  we know it must be an H2 URL by that point in the code
* Introduce private isNotH2DataStore method because I just don't
  like reading !someThing(...) and prefer to see isNotSomeThing(...)
  in the main logic
* Update javadocs to match new logic
* IntelliJ reformatted some code, and I let it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant