Conversation
Percent-encoded sqlite:// paths were stored as literal %20 filenames on Windows, unlike file:// artifacts. Parse drive-letter URIs with url2pathname so session DBs land on the intended path.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
On Windows,
SqliteSessionServiceand the CLIsqlite://factory do not treat drive-letter session URIs the wayfile://artifact URIs already do.urlparse("sqlite:///C:/tmp/adk%20sessions.db").pathstays'/C:/tmp/adk%20sessions.db'(percent-encoding is not decoded byurlparse). The factory only stripped a leading/, so--session_service_uricreated a database whose filename still contained literal%20. The same helper also rejected the documented SQLAlchemy Windows form when the official unit test builtsqlite+aiosqlite:////+C:\...(lstrip('/')does nothing on a drive path), sotest_sqlite_session_service_accepts_absolute_sqlite_urlsfailed on Windows.Steps to Reproduce:
uv venv --python 3.11 .venvanduv sync.pytest tests/unittests/sessions/test_session_service.py::test_sqlite_session_service_accepts_absolute_sqlite_urls.--session_service_uri sqlite:///C:/Users/Someone/My%20Docs/sessions.dbtoadk web.Expected Behavior:
sqlite:///C:/path/to.db(and a percent-encoded space) opens that filesystem path, matching SQLAlchemy and the existingfile://artifact factory.Observed Behavior:
The absolute-URL unit test fails because the constructed URI is not a valid Windows sqlite URL. A percent-encoded CLI URI creates
adk%20sessions.dbinstead ofadk sessions.db.Environment Details:
main@7b246e0Solution:
Reuse
unquote+ Windowsurl2pathnamefor drive-letter sqlite URL paths inside_parse_db_path(including the four-slash//C:/...shape). Point the CLI sqlite factory at that helper so it cannot drift fromSqliteSessionService. Leave relative URIs (sqlite:///test.db) as relative paths so they do not become\test.db. Build the official absolute-URL test withPath.as_posix()so Unix stays////tmp/...and Windows usessqlite:///C:/path/to.db.User impact
Windows users can persist sessions under a drive-letter path, including directories that contain spaces, without getting a differently named database file. The Unix relative and four-slash absolute contracts are unchanged.
Testing Plan
Unit Tests:
Manual End-to-End (E2E) Tests:
N/A for this parser fix. The new Windows test creates a real sqlite file at
tmp_path / 'adk sessions.db'fromsqlite+aiosqlite:///C:/.../adk%20sessions.dband asserts the percent-encoded filename is not created.Checklist
Additional context
file://artifact URIs already callunquote+url2pathnameon Windows. This change brings sqlite session URIs to the same rule. I have signed / will sign the Google CLA as required by CONTRIBUTING.