O365 / python-o365

A simple python library to interact with Microsoft Graph and Office 365 API
Apache License 2.0
1.69k stars 424 forks source link

Connection: Should Client Error be logged as debug instead of error? #1080

Open dekiesel opened 6 months ago

dekiesel commented 6 months ago

Hi,

I am using drive.get_item_by_path() to check which files a folder contains, if any.

If a folder doesn't exist I consider it to be empty.

            try:
                remote_folder = default_drive.get_item_by_path( 
                   "my/nonexistant/path"
                )
            except HTTPError as he:
                if "Resource not found for the segment" in repr(he):
                    return []

The issue is that connection.py (line 838) logs the exception. Since there is a use-case where this is the intended behaviour and not actually an unexpected error would you be open to reducing the log-level to debug?

alejcas commented 6 months ago

Asking for a folder that doesn't exist results in a 4XX http code. Calling a ms graph endpoint with incorrect parameters will also return a 4XX http code.

Do you have the complete http code returned when a folder doesn't exist?

maybe something can be donde there, but if it's too generic I don't think I can change the log to debug.

dekiesel commented 6 months ago

Client Error: 400 Client Error: Bad Request for url: https://graph.microsoft.com/v1.0//sites/mytenant.sharepoint.com,ba805102-49d8-47ab-8aad-7d14db214a60,e19ca39c-5aa9-4639-9b54-7966cd9c088b/drive/root:my/test/root/a/b/c | Error Message: Resource not found for the segment 'root:my'. | Error Code:

                if status_code == 4 and "Resource not found for the segment" not in repr(e):
                    # Client Error
                    # Logged as error. Could be a library error or Api changes
                    log.error(
                        "Client Error: {} | Error Message: {} | Error Code: {}".format(
                            str(e), error_message, error_code
                        )
                    )

Could be a solution, but I could see while you'd be hesitant. If you decide against it I can always exclude o365 from the root logger.

alejcas commented 6 months ago

400 is extremely generic...

While I can certainly do what you paste you can see the snowball effect this can have.

Let me think about it.

dekiesel commented 6 months ago

In case this changes your decision:

while creating pr #1081 I noticed that the client error isn't actually 400, but 404.

404 Client Error: Not Found for url: https://graph.microsoft.com/v1.0//sites/mysite.sharepoint.com,ba872102-39d8-47ab-8aad-7d14db214b60,e18ca39c-5ac9-4639-9b54-7966cc9c088b/drive/root:/myfiles/i/dont/exist | Error Message: The resource could not be found.

I got 400 because something was actually wrong with my query, not because the folder doesn't exist.