QueryBuilder: fix type string filter generation for Group subclasses

[sphuber]

Fixes #4143

The aiida.orm.querybuilder.get_group_type_filter function that is
responsible for generating the correct filters for the type_string
when a Group or subclass of it is appended, contained a bug. To
generate the correct type string, it should strip the group. prefix
from the provided ORM classifier. This was incorrectly done with the
string method lstrip, which does not strip an entire prefix as is, but
will strip any characters that are matched starting from the left. This
would cause extra characters to be erroneously stripped if the rest of
the type string started with any letters in the sequence group.. For
example group.pseudo.family would be stripped to seudo.family. The
solution is to use the first N characters where N is the length of the
prefix.

[codecov]

Codecov Report

Merging #4144 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4144      +/-   ##
===========================================
+ Coverage    78.86%   78.87%   +0.02%     
===========================================
  Files          467      467              
  Lines        34481    34481              
===========================================
+ Hits         27189    27194       +5     
+ Misses        7292     7287       -5     

Flag	Coverage Δ
#django	70.79% <100.00%> (ø)
#sqlalchemy	71.68% <100.00%> (+0.02%)	⬆️

Impacted Files	Coverage Δ
aiida/orm/querybuilder.py	80.00% <100.00%> (ø)
aiida/orm/utils/log.py	93.34% <0.00%> (+16.67%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 690025e...b7bacd5. Read the comment docs.

[ramirezfranciscof]

My only question would be if by this point in the code is it guaranteed that the string will start with that prefix and you are not just stripping part of the type away? And this is backwards compatible with older versions of this label?

[sphuber]

My only question would be if by this point in the code is it guaranteed that the string will start with that prefix and you are not just stripping part of the type away?

Yes, this should be guaranteed because the QueryBuilder is also responsible for prepending the prefix elsewhere:

aiida-core/aiida/orm/querybuilder.py

Line 97 in 690025e

classifiers['ormclass_type_string'] = GROUP_ENTITY_TYPE_PREFIX + cls._type_string

The code is a mess overall, but it will require a lot of work to refactor it all in to something more comprehensible and less error prone.

And this is backwards compatible with older versions of this label?

There is no question of backwards compatibility here. This was an internal bug in the QueryBuilder logic that was introduced in 1.2.0 when support for Group subclassing was added. This should not affect any user code, except solve a bug

[ramirezfranciscof]

There is no question of backwards compatibility here. This was an internal bug in the QueryBuilder logic that was introduced in 1.2.0 when support for Group subclassing was added. This should not affect any user code, except solve a bug

No, what I meant is that perhaps this was using lstrip to take into account the possibility of the user having older labels that may not include the group. prefix, but I guess that if you say this is not intrinsecal to the group but added by the querybuilder then the point is moot.

[ramirezfranciscof]

Saul Goodman!

[sphuber]

No, what I meant is that perhaps this was using lstrip to take into account the possibility of the user having older labels that may not include the group.

It was using lstrip because I was being a Python-amateur 😅 .

Thanks for the review 👍

[greschd]

It was using lstrip because I was being a Python-amateur 😅 .

That is common enough that it got its own PEP: https://www.python.org/dev/peps/pep-0616/ 😂

[sphuber]

Python 3.9 here we go ! 🚀