Re: [PR] feat(bug): defensive code to avoid accesing attribute of a NoneType object [superset]

[via GitHub]

korbit-ai[bot] commented on code in PR #35219:
URL: https://github.com/apache/superset/pull/35219#discussion_r2364604727


##########
superset/dashboards/api.py:
##########
@@ -1342,6 +1342,8 @@ def thumbnail(self, pk: int, digest: str, **kwargs: Any) 
-> WerkzeugResponse:
         self.incr_stats("from_cache", self.thumbnail.__name__)
         try:
             image = cache_payload.get_image()
+            if not image or not hasattr(image, "read"):
+                return self.response_404()

Review Comment:
   ### Overly restrictive hasattr check for read attribute <sub>![category 
Functionality](https://img.shields.io/badge/Functionality-0284c7)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   The defensive check uses `hasattr(image, "read")` which can return False for 
valid file-like objects that have a `read` attribute but it's not accessible 
due to various reasons (e.g., closed file, permission issues), potentially 
causing false positives.
   
   
   ###### Why this matters
   This could incorrectly return 404 for valid images that temporarily don't 
have accessible read attributes, leading to unnecessary failures when the image 
could be successfully processed.
   
   ###### Suggested change ∙ *Feature Preview*
   Use a more robust check that attempts to verify the object is file-like by 
checking if it's callable or use duck typing with a try-except block:
   
   ```python
   if not image or not (hasattr(image, "read") and callable(getattr(image, 
"read", None))):
       return self.response_404()
   ```
   
   Or alternatively:
   
   ```python
   if not image:
       return self.response_404()
   try:
       # Verify the read method exists and is callable
       if not callable(getattr(image, "read", None)):
           return self.response_404()
   except AttributeError:
       return self.response_404()
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1aaa6640-d688-4d12-a1f0-6ff5d2272b28/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1aaa6640-d688-4d12-a1f0-6ff5d2272b28?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1aaa6640-d688-4d12-a1f0-6ff5d2272b28?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1aaa6640-d688-4d12-a1f0-6ff5d2272b28?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1aaa6640-d688-4d12-a1f0-6ff5d2272b28)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:ffa63663-843b-453c-8ec6-5461065ba3e5 -->
   
   
   [](ffa63663-843b-453c-8ec6-5461065ba3e5)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org