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

Add schema name option to database introspection methods #1466

Merged
merged 9 commits into from
Jan 6, 2024

Conversation

barnettben
Copy link
Collaborator

Adds an optional schemaName parameter to table introspection methods as discussed in #1463.

I have added the extra parameter to foreignKeyViolations() since that does support checking in other schemas - it then follows to also update checkForeignKeys(). I'm not totally happy with the doc comments on checkForeignKeys(), but I think they makes sense.

Commits in this PR are mostly grouped by which function they're updating, so are hopefully easy to review.

I have not updated the sample code in the README since the code in there still works and doesn't really need to show this optional addition.

One problem is that make smokeTest persistently fails on DatabaseCursorTests.testFastDatabaseValueCursorStep(). This is the first profiling test to run and takes about 3x longer than others in the same file. It appears unrelated to any of the code that I've touched with this PR. Other tests all pass.

Any and all critiques welcome!


Pull Request Checklist

  • CONTRIBUTING: You have read https://github.com/groue/GRDB.swift/blob/master/CONTRIBUTING.md
  • BRANCH: This pull request is submitted against the development branch.
  • DOCUMENTATION: Inline documentation has been updated.
  • DOCUMENTATION: README.md or another dedicated guide has been updated.
  • TESTS: Changes are tested.
  • TESTS: The make smokeTest terminal command runs without failure.

@groue
Copy link
Owner

groue commented Dec 16, 2023

Thank you @barnettben, it looks like a perfect PR :-)

I have not updated the sample code in the README since the code in there still works and doesn't really need to show this optional addition.

👍

One problem is that make smokeTest persistently fails on DatabaseCursorTests.testFastDatabaseValueCursorStep(). This is the first profiling test to run and takes about 3x longer than others in the same file. It appears unrelated to any of the code that I've touched with this PR. Other tests all pass.

Yes, it is unrelated, so it's ok to ignore it here. Do you know how it fails?


I'll help fixing the CI builds. Please stay tuned.

@barnettben
Copy link
Collaborator Author

Thank you @groue.
Looks like I mis-read which test was failing. It is actually testNullableDatabaseValueCursorForEach().

The log shows:

Thrown Error at SerializedDatabase.swift:125:failed: caught error: "SQLite error 10: disk I/O error - while executing `PRAGMA query_only = 0`" (0.00s)

I have not looked in to it further yet. Hopefully it's not the symptom of a failing SSD!

@groue
Copy link
Owner

groue commented Dec 20, 2023

I'm a little busy these days, but I'm in vacations at the end of the week. I should be able to look at the PR, then.

@barnettben
Copy link
Collaborator Author

Thank you, I'm not in a hurry. Don't spoil your holiday!

@groue
Copy link
Owner

groue commented Jan 6, 2024

Hi @barnettben,

I have skipped tests that involve attached databases when they run with SQLCipher + encryption. Another option would be to provide the password to the ATTACH statement. But this would just make the test more complicated without much testing benefit. SQLCipher is still tested (when not encrypted).

Another CI problem was this error message:

Run kishikawakatsumi/xcresulttool@v1
Error: Resource not accessible by integration

I have tried the techniques described in https://github.com/orgs/community/discussions/60820. Now I wait to see if CI is happier 😅

EDIT: nope, I could not solve the "Resource not accessible by integration" error. I just disabled kishikawakatsumi/xcresulttool, which wasn't very useful at reporting CI test failures anyway.

@groue
Copy link
Owner

groue commented Jan 6, 2024

All CI tests pass 🎉 Merging

@groue groue merged commit ac8d1b7 into groue:development Jan 6, 2024
21 checks passed
@groue groue changed the title Add schema name option to table introspection methods Add schema name option to database introspection methods Jan 6, 2024
@groue
Copy link
Owner

groue commented Jan 6, 2024

Shipped in v6.24.0 👍

Thank you very much @barnettben. Would you accept an invitation that grants you a push access to GRDB? I do intend to keep the lead of the project, but it is good for everyone that people of various levels of expertise can improve the repository. Of course, your free time would remain just as free as it was before!

@barnettben
Copy link
Collaborator Author

Thank you @groue, I'd be happy to accept. And thanks for merging this in too.

@groue
Copy link
Owner

groue commented Jan 8, 2024

You're very welcome, @barnettben 🙂🤝

@groue groue mentioned this pull request Jan 29, 2024
@barnettben barnettben deleted the 1463-schema-names branch November 16, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants