Skip to content

Conversation

renaudll
Copy link
Contributor

Fix crash when uploading a thumbnail.

$ rez-env python-3.11 sq_shotgun shotgun_api3-3.8.2  -- python -c 'import sq_shotgun; sg = sq_shotgun.connect("Toolkit");  sg.upload("PublishedFile", 123456, "/path/to/no_preview.jpg", field_name="thumb_image")'
Traceback (most recent call last):
(...)
  File "/home-local/rlarouche/packages/shotgun_api3/3.8.2/python/shotgun_api3/shotgun.py", line 4665, in _send_form
    raise ShotgunError("Max attemps limit reached.")
shotgun_api3.shotgun.ShotgunError: Max attemps limit reached.

I've isolated the regression to shotgun_api3-3.8.2, more specifically this commit:

https://github.com/shotgunsoftware/python-api/pull/368/files?diff=split&w=0

The issue come from the change in the parent class of CACertsHTTPSHandler.
It was changed from HTTPSHandler to HTTPHandler.
This change is weird as the class re-implement https_open which don't exist in HTTPHandler.

I assume this was done because without changing the parent class, we get the following error:

  File "/squeeze/rez_software/rez_package/python/3.11.9/platform-linux/arch-x86_64/os-Rocky-9/lib/python3.11/http/client.py", line 1000, in send
    if self.debuglevel > 0:
       ^^^^^^^^^^^^^^^^^^^
TypeError: '>' not supported between instances of 'CACertsHTTPSHandler' and 'int'

The reason we get this error is because of how some lines where converted to super calls.

They were converted from:

urllib.request.HTTPSHandler.__init__(self)

To:

super().__init__(self)

But they should be:

super().__init__()

Passing self to HTTPSHandler.__init__ will set the debuglevel keyword argument, hence the crash.

I hope this helps!

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

In this code snippet, the handling of custom CA certificates is clearly the main focus, and there are a few points worth noting for the review. The __init__ method correctly extracts the ca_certs argument from kwargs before calling the superclass initializer, which ensures that unexpected keyword arguments are not passed up the chain. However, it would be good to add error handling or a default value when popping ca_certs, since its absence could lead to a KeyError and potentially break instantiation. In the connect method, the logic for wrapping the HTTP socket with SSL is clean, but the conditional check on six.PY38 might benefit from a more explicit comment explaining why Python 3.8 requires a different approach, as this would help future maintainers avoid unintentional regressions. For the CACertsHTTPSHandler class, the use of a custom HTTPS handler to enforce CA certs is a solid design choice, but it would be beneficial to include parameter type hints in __init__ for cacerts (e.g., str or pathlib.Path) to clarify expectations. Lastly, while https_open currently delegates to the superclass method, adding a short comment about how this method uses the stored __ca_certs would improve readability for others reading the code.

@renaudll
Copy link
Contributor Author

@carlos-villavicencio-adsk since this is related to your commit would you mind taking a quick look?

Thanks!

@carlos-villavicencio-adsk carlos-villavicencio-adsk changed the title Fix misused super in CACertsHTTPSConnection and CACertsHTTPSHandler SG-40026 Fix misused super in CACertsHTTPSConnection and CACertsHTTPSHandler Aug 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants