fix: sub module conflict error

[StellarisW]

Currently the logic of register sub module to sys.modules may cause conflict,

That's because of the PR #249 which fix #215

def load(path,
         module_name=None,
         include_dirs=None,
         include_dir=None,
         encoding='utf-8'):

    real_module = bool(module_name)
    thrift = parse(path, module_name, include_dirs=include_dirs,
                   include_dir=include_dir, encoding=encoding)
    if threadlocal.incomplete_type:
        fill_incomplete_ttype(thrift, thrift)

    if real_module:
        sys.modules[module_name] = thrift
        sub_modules = thrift.__thrift_meta__["includes"][:]
        while sub_modules:
            module = sub_modules.pop()
            if module not in sys.modules:
                sys.modules[module.__name__] = module
                sub_modules.extend(module.__thrift_meta__["includes"])
    return thrift

For example:

We have thrift files as follows:

thrift files
├── idl
│   ├── main.thrift
│   ├── included1.thrift
│   ├── included2.thrift

If we call load('idl/main.thrift', module_name='idl.main_thrift'), then the following modules will be registered:

• idl.main_thrift
• included1_thrift
• included2_thrift

So, if included1.thrift is a common module name like platform, which is platform.thrift, then module platform will register to sys.modules, so it will cause conflict.

Now I come up with 2 solutions:

1. Add an option to load(), which will be def load(path, module_name=None, include_dirs=None, include_dir=None, encoding='utf-8'), and let the users to register sub modules manually. (not recommended)

2. Optimize sub module name. (recommended)

   use absolute path as its module name instead of the base name of the thrift file.

   let's take the example before, then the following modules will be registered:

   • idl.main_thrift
   • idl.included1_thrift
   • idl.included2_thrift

[StellarisW]

Please take a review💌
@aisk @ethe

[aisk]

Sorry, I can't access my development computer these days. I will review it a few days later.

But the test case should be passed before merging.

[StellarisW]

Sorry, I can't access my development computer these days. I will review it a few days later.

But the test case should be passed before merging.

That's OK, I'll fix the test cases.

[cocolato]

just use s.replace(suffix, "") is better.

import itertools

from faker import Faker 

SUFFIX = "__SOME_SUFFIX_"

class MyFaker(Faker):
    def suffix_name(self):
        return f"{self.name()}{SUFFIX}"

fake = MyFaker()
data_num = 10000
escape_ratio = 0.5
escape_num = int(data_num * escape_ratio)

suffix_texts = [fake.suffix_name() for _ in range(escape_num)]
normal_texts = [fake.name() for _ in range(data_num - escape_num)]

def test_and_replace():
    for text in itertools.chain(suffix_texts, normal_texts):
        if text.endswith(SUFFIX):
            res = text.replace(SUFFIX, "")
    return res

def test_and_replace_with_slie():
    for text in itertools.chain(suffix_texts, normal_texts):
        if text.endswith(SUFFIX):
            res = text[:-len(SUFFIX)]
    return res

def just_replace():
    for text in itertools.chain(suffix_texts, normal_texts):
        res = text.replace(SUFFIX, "")
    return res


__benchmarks__ = [
    (test_and_replace, just_replace, "just_replace"),
    (test_and_replace, test_and_replace_with_slie, "test_and_replace_with_slie")
]

                                         Benchmarks, repeat=5, number=20                                          
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃                  Benchmark ┃ Min     ┃ Max     ┃ Mean    ┃ Min (+)         ┃ Max (+)         ┃ Mean (+)        ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│               just_replace │ 0.043   │ 0.043   │ 0.043   │ 0.021 (2.0x)    │ 0.022 (1.9x)    │ 0.022 (2.0x)    │
│ test_and_replace_with_slie │ 0.041   │ 0.042   │ 0.042   │ 0.040 (1.0x)    │ 0.041 (1.0x)    │ 0.040 (1.0x)    │
└────────────────────────────┴─────────┴─────────┴─────────┴─────────────────┴─────────────────┴─────────────────┘

[StellarisW]

Thanks for the advice, it seems better indeed.

[cocolato]

Suggested change

	sub_modules = thrift.__thrift_meta__["sub_modules"][:]
	for module in sub_modules:
	if module not in sys.modules:
	sys.modules[module.__name__] = include_thrift
	lost_modules = [
	m for m in thrift.__thrift_meta__["sub_modules"] if m not in sys.modules
	]
	for module in lost_modules:
	sys.modules[module.__name__] = include_thrift

[aisk]

These code's indents are a little bit strange, I think it's better to using something like:

aaa = bbb().ccc()\
    .ddd()

[aisk]

Thank you for your work, but I still have a question.

Although the included Thrift modules wouldn't overwrite each other when they have the same name but are in different subdirectories after merge this PR, we support loading a Thrift file with a dot in its name, such as foo.bar.thrift. This means that if there is both a foo/bar.thrift and a foo.bar.thrift, they might still overwrite each other.

I think the expected behavior should be to raise an exception to inform users about the conflict. I believe the current code already has this issue, but the new code might have the potential to address it.

[StellarisW]

Thank you for your work, but I still have a question.

Although the included Thrift modules wouldn't overwrite each other when they have the same name but are in different subdirectories after merge this PR, we support loading a Thrift file with a dot in its name, such as foo.bar.thrift. This means that if there is both a foo/bar.thrift and a foo.bar.thrift, they might still overwrite each other.

I think the expected behavior should be to raise an exception to inform users about the conflict. I believe the current code already has this issue, but the new code might have the potential to address it.

Okay, I'll raise an exception in this case.

[StellarisW]

@aisk All things have done, but there is still a test fail in def test_load_fp(), it because of the load_fp() uses threadlocal.thrift_cache which has been set in other test, don't know what is the best way to fix it.

[cocolato]

It would be good if you could squash all the commits down to one, just to tidy up the history.

[StellarisW]

It would be good if you could squash all the commits down to one, just to tidy up the history.

done

[StellarisW]

@aisk Can you please take a review, this PR is delayed quite a long time, thank you !

[le119126]

I also meet the cause，waiting

[cocolato]

@aisk All things have done, but there is still a test fail in def test_load_fp(), it because of the load_fp() uses threadlocal.thrift_cache which has been set in other test, don't know what is the best way to fix it.

try this

from thriftpy2.parser import threadlocal
threadlocal.__dict__.clear()

[StellarisW]

@aisk All things have done, but there is still a test fail in def test_load_fp(), it because of the load_fp() uses threadlocal.thrift_cache which has been set in other test, don't know what is the best way to fix it.

try this

from thriftpy2.parser import threadlocal
threadlocal.__dict__.clear()

Thanks, all tests have passed right now

[aisk]

I found that this empty module is only used to check for conflicts in sys.modules by using it's name. Maybe we can make the code more readable by assigning the attribute __thrift_module_name__ to the module (the variable child here)?

[aisk]

I'd like to add a new exception ThriftModuleNameConflict which inherits ThriftParserError to make it easier to catch.

[StellarisW]

fixed, please review the code again💌