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

Fix the wrong validator of ModulemdUnit.profiles [RHELDST-22116] #217

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

lebouillon
Copy link
Contributor

Currently, the wrong validator resulted in a TypeError, saying pubtools-pulplib is validating the after-conversion "profiles" field as a dict:

TypeError: "'profiles' must be <class 'dict'>
(got frozendict.frozendict({'test': ['secondRepoRPM']}) that is a <class 'frozendict.frozendict'>)."

The validator of ModulemdUnit.profiles should be a frozendict rather than a dict after the "converter=frozenlist_or_none_converter" conversion.

Currently, the wrong validator resulted in a TypeError, saying
pubtools-pulplib is validating the after-conversion "profiles" field as
a dict:

TypeError: "'profiles' must be <class 'dict'>
(got frozendict.frozendict({'test': ['secondRepoRPM']})
that is a <class 'frozendict.frozendict'>)."

The validator of ModulemdUnit.profiles should be a frozendict rather
than a dict after the "converter=frozenlist_or_none_converter"
conversion.
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ddac319) 100.00% compared to head (4667b0c) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #217   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         3162      3162           
=========================================
  Hits          3162      3162           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rohanpm
Copy link
Member

rohanpm commented Dec 6, 2023

There seems to be a deeper story behind this issue. I find it hard to believe that the bug could be as described because it hasn't changed recently and how could it possibly not have broken anything until now?

Furthermore, these validators are documented as using "isinstance", so when we say optional_dict we're not saying the value has to be precisely a dict. It's allowed to be any subclass of dict, and I would expect frozendict to be a subclass of dict, so it should have been working fine.

Could you check the following in whatever environment you could reproduce this? For me, frozendict is an instance of dict so there is no problem with the validator.

>>> from frozendict.core import frozendict
>>> isinstance({}, dict)
True
>>> isinstance(frozendict({}), dict)
True

@rohanpm
Copy link
Member

rohanpm commented Dec 6, 2023

...and I do have a vague recollection that frozendict has both a C and a pure-python implementation, and if you end up using the C implementation it's not a subclass of dict, while if you use the pure-python implementation it is.

@lebouillon
Copy link
Contributor Author

lebouillon commented Dec 6, 2023

Could you check the following in whatever environment you could reproduce this? For me, frozendict is an instance of dict so there is no problem with the validator.

@rohanpm Sure. And it approved your assumption that environment is the cause.

I am running on Python 3.9.17 with frozendict 2.3.10
>>> from frozendict.core import frozendict
>>> isinstance({}, dict)
True
>>> isinstance(frozendict({}), dict)
False

But it seems both cannot allow the subclass to be True.

Here is a isinstance(frozendict({}), dict) check for the C Extension.

(python39) [dichen@arpeggio ~]$ CIBUILDWHEEL=1 pip install frozendict
Collecting frozendict
  Using cached frozendict-2.3.10-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (115 kB)
Installing collected packages: frozendict
Successfully installed frozendict-2.3.10

[notice] A new release of pip is available: 23.0.1 -> 23.3.1
[notice] To update, run: pip install --upgrade pip
(python39) [dichen@arpeggio ~]$ python
Python 3.9.17 (main, Jun  8 2023, 00:00:00) 
[GCC 13.1.1 20230511 (Red Hat 13.1.1-2)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from frozendict.core import frozendict
>>> isinstance(frozendict({}), dict)
False

Here is the isinstance(frozendict({}), dict) check for the pure py implementation.

(python39) [dichen@arpeggio ~]$ FROZENDICT_PURE_PY=1 pip install frozendict
Collecting frozendict
  Using cached frozendict-2.3.10-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (115 kB)
Installing collected packages: frozendict
Successfully installed frozendict-2.3.10

[notice] A new release of pip is available: 23.0.1 -> 23.3.1
[notice] To update, run: pip install --upgrade pip
(python39) [dichen@arpeggio ~]$ python
Python 3.9.17 (main, Jun  8 2023, 00:00:00) 
[GCC 13.1.1 20230511 (Red Hat 13.1.1-2)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from frozendict.core import frozendict
>>> isinstance(frozendict({}), dict)
False

I will keep doing some investigation for frozendict subclass issue.
PS: This same frozedict issue caused a hinderance for release-engineering/ubi-manifest#68 (comment)

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

I suppose we should still submit this despite that an issue outside of this library triggers the problem. It should work fine as long as we ensure that the converter and the validator are using the same type (i.e. either both using the pure-python frozendict or both using the C extension frozendict).

@rohanpm rohanpm merged commit f1a9579 into release-engineering:master Dec 7, 2023
8 checks passed
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