Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dekiesel opened this issue May 30, 2024 · 4 comments
Open

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

dekiesel opened this issue May 30, 2024 · 4 comments

Comments

@dekiesel
Copy link

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
Copy link
Member

alejcas commented May 30, 2024

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
Copy link
Author

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
Copy link
Member

alejcas commented May 30, 2024

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
Copy link
Author

dekiesel commented May 31, 2024

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.

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

No branches or pull requests

2 participants