fix: mutable reference headers #1095

[AsahiSoftWareEngineer]

What kind of change does this PR introduce?

Bug fix Isses #1095

What is the current behavior?

Isses No. #1095

What is the new behavior?

Prevent unintended header changes when sharing the same ClientOptions.

[AsahiSoftWareEngineer]

@olirice Sorry, I can not set reviewer. That's why , I request a review here.
Please review my pull request.

[coveralls]

Pull Request Test Coverage Report for Build 15212706024

Details

• 9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.4%) to 86.318%

---

Totals
Change from base Build 14719173473:	0.4%
Covered Lines:	347
Relevant Lines:	402

---

💛 - Coveralls

[grdsdev]

Thanks for the fix @AsahiSoftWareEngineer just some clarifications before merging.

[grdsdev]

not sure the issue this is solving, could you elaborate?

[AsahiSoftWareEngineer]

Sorry, Below source code is not necessary.

  self.postgrest.session.headers["Authorization"] = header
  self.storage.session.headers["Authorization"] = header

Caused by using deep-copy, before source code was not working. (auth._headers is not refreshed).
That's why, I changed to reassign deep-copied self._create_auth_header(access_token)methods

[AsahiSoftWareEngineer]

@grdsdev Sorry, I fixed it. Please re-review pull request.

[silentworks]

I think this would be better addressed by using the .replace method on the ClientOptions class. This should probably be added to the docs as the go to way of updating anything inside of the ClientOptions.

I wrote your test below using the .replace method and it all passes.

def test_mutable_headers_issue():
    url = os.environ.get("SUPABASE_TEST_URL")
    key = os.environ.get("SUPABASE_TEST_KEY")

    shared_options = ClientOptions(
        headers={"Authorization": "Bearer initial-token", "x-site": "supanew.site"}
    )

    client1 = create_client(url, key, shared_options)
    client2 = create_client(url, key, shared_options)
    client1.options.replace({"headers": {"Authorization": "Bearer modified-token"}})

    assert client2.options.headers["Authorization"] == "Bearer initial-token"
    assert client2.options.headers["x-site"] == "supanew.site"
    assert client1.options.headers["x-site"] == "supanew.site"

[AsahiSoftWareEngineer]

@silentworks Sure. I think of trying, but I do not know Can I rewrite docs.(Have I permissions??)

[AsahiSoftWareEngineer]

All test passed

[AsahiSoftWareEngineer]

@grdsdev Hi, Can you re-review pull request.

[grdsdev]

I think this would be better addressed by using the .replace method on the ClientOptions class. This should probably be added to the docs as the go to way of updating anything inside of the ClientOptions.

I wrote your test below using the .replace method and it all passes.

def test_mutable_headers_issue():
    url = os.environ.get("SUPABASE_TEST_URL")
    key = os.environ.get("SUPABASE_TEST_KEY")

    shared_options = ClientOptions(
        headers={"Authorization": "Bearer initial-token", "x-site": "supanew.site"}
    )

    client1 = create_client(url, key, shared_options)
    client2 = create_client(url, key, shared_options)
    client1.options.replace({"headers": {"Authorization": "Bearer modified-token"}})

    assert client2.options.headers["Authorization"] == "Bearer initial-token"
    assert client2.options.headers["x-site"] == "supanew.site"
    assert client1.options.headers["x-site"] == "supanew.site"

@silentworks I just think I prefer to internally deep copy the client options object, I think that just makes more sense then having to use a separate replace method for changing the options for the client.

[silentworks]

@grdsdev this is fine. I do however think we are trying to fix a language (python being dynamic by design) issue here but I am OK with this PR being merged in.

[grdsdev]

@grdsdev this is fine. I do however think we are trying to fix a language (python being dynamic by design) issue here but I am OK with this PR being merged in.

that makes sense, let's not fight the language.

[grdsdev]

@AsahiSoftWareEngineer thank you for this contributions, but as said by @silentworks there're other ways of achieving this by using the replace method.

[silentworks]

We're re-opening this PR as I've realised in my tests I didn't test it fully. .replace doesn't work as how I had initially thought as it returns a new object. In order to use .replace and get the same result as this PR the code would look more like the test below which I think might be too verbose for some.

def test_mutable_headers_issue():
    url = os.environ.get("SUPABASE_TEST_URL")
    key = os.environ.get("SUPABASE_TEST_KEY")

    shared_options = ClientOptions(
        headers={"Authorization": "Bearer initial-token", "x-site": "supanew.site"}
    )

    client1 = create_client(url, key, shared_options)
    client2 = create_client(url, key, shared_options)
    client1.options = client1.options.replace(headers={**client1.options.headers, "Authorization": "Bearer modified-token"})

    assert client1.options.headers["Authorization"] == "Bearer modified-token"
    assert client2.options.headers["Authorization"] == "Bearer initial-token"
    assert client2.options.headers["x-site"] == "supanew.site"
    assert client1.options.headers["x-site"] == "supanew.site"