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

Using serializable.serializable_class during import configures root logger #9

Closed
kroeschl opened this issue Aug 28, 2023 · 3 comments · Fixed by #47
Closed

Using serializable.serializable_class during import configures root logger #9

kroeschl opened this issue Aug 28, 2023 · 3 comments · Fixed by #47
Labels
enhancement New feature or request

Comments

@kroeschl
Copy link
Contributor

kroeschl commented Aug 28, 2023

Description

If some user of py-serializable calls serializable.serializable_class before configuring the root logger (which is common during import blocks), the root logger ends up configured with the default basic configuration.

This happens because we call logging.debug in ObjectMetadataLibrary.register_klass. Calling logging.debug (or similar) will automatically call logging.basicConfig if no handlers are configured for the root logger.

We should probably do what most modules do and set up our own logger and NullHandler rather than using the root logger.

Example

import logging
import serializable

@serializable.serializable_class
class Foo:
    pass

_logger = logging.getLogger()
_logger.addHandler(logging.NullHandler())
_logger.error("foo")

Outputs ERROR:root:foo, which shouldn't happen if I only configure a NullHandler.

Workaround

I'm currently working around this by wrapping the import of anything that uses py-serializable (in this case, CycloneDX) in a logging configuration hack:

_root_logger = logging.getLogger()
_root_null_handler = logging.NullHandler()
_root_logger.addHandler(_root_null_handler)
from cyclonedx.model.bom import Bom
_root_logger.removeHandler(_root_null_handler)

Version Info

  • Python 3.10.9 & Python 3.9.13
  • py-serializable 0.11.1
@kroeschl
Copy link
Contributor Author

I'll post a PR to fix this if the issue is approved or sits for a while.

@bmihaila-bd
Copy link

Ran into same issue today after spending time to find out where it happens. Issue is as described above in #9 (comment) - the library calls logging.debug() instead of using logger.debug(). As the serialization library is used via annotations the code is called on imports of any other code using it.

Another workaround for reconfiguring the root logger after the import of madpah/serializable/serializable/init.py is to use logging.basicConfig(..., force=True) and overwrite the settings later. However, the proper fix should be done in this library by not using the global logging.debug calls.

@kroeschl please go ahead for a PR to get this fixed

@jkowalleck jkowalleck added the enhancement New feature or request label Sep 27, 2023
@jkowalleck
Copy link
Collaborator

@kroeschl @bmihaila-synopsys

I took solution #10 and modified it to all needs. Now it looks good to me.
Please have a last review, before it gets merged and published :-)

@jkowalleck jkowalleck mentioned this issue Dec 19, 2023
@jkowalleck jkowalleck changed the title Using serializable.serializable_class during import configures root logger Using serializable.serializable_class during import configures root logger Dec 27, 2023
@jkowalleck jkowalleck pinned this issue Dec 27, 2023
@jkowalleck jkowalleck unpinned this issue Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants