refactor(server/config): Implement checked access to server configuration, decoupled from SessionManager

[whisperity]

This patch introduces the configuration_access library which offers semantically correct and higher-level access to a configuration data structure, via Options as defined in a Schema, as opposed to raw operator[] of random dicts.

Options are registered into an ongoing Schema instance mimicking the interface of standard argparse. An Option carries with it potential semantic enhancements such as a validation function to automatically detect invalid configuration values, or an encapsulated default value.

Refactored the CodeChecker server package to use the new logic for accessing the [$CONFIG_DIRECTORY/]server_config.json file. Originally, the CodeChecker server only came with command-line options, which was extended with a configuration file for authentication purposes in #393. However, #1249 added a serious design blunder, in which the previous session_config.json was mixed together with other non–authentication-related options and became server_config.json. Unfortunately, the changing of the file's "schema" also came with hacking access to non–authentication-related configuration fields into the SessionManager class.

Moving forward with the development of the asynchronous store protocol is blocked by this lack of this separation of concerns, as background worker threads should not be given access to authentication data just for them to access otherwise needed configuration values. Accessing of shared state between different pools without explicit IPC primitives and exclusion can very quickly deadlock the system.

[vodorok]

I am not finished with the review, but I think the first round of questions and notes are worth to publish.

[vodorok]

Why are you searching both of the requirements.txts:?

[whisperity]

This patch updated codechecker_common's dev-requirements with new dependencies, and those new dependencies are needed to be installed for the static analysis to be run in this linting job. The "common tests" now runs only the unit tests for the new code added to codechecker_common. Having static analysis (mypy) run together with the other linters is more appropriate than making the hard test case checking fail because mypy overapproximated something.

[vodorok]

Could you please further divide the test methods?

[vodorok]

Why reversed?

[whisperity]

pathlib.PurePath.parents returns a generator of parents from inside out, starting with the most direct parent first. However, in order to traverse the mapping we need to go outside in, starting from the outermost parent. Saying "/foo/bar is not defined" (think like "not a directory") when even /foo is missing follows the allegory with file systems and tree hierarchies.

>>> import pathlib
>>> p = pathlib.PurePosixPath("/foo/bar/baz/qux/aaa.txt")
>>> list(p.parents)
[PurePosixPath('/foo/bar/baz/qux'), PurePosixPath('/foo/bar/baz'), PurePosixPath('/foo/bar'), PurePosixPath('/foo'), PurePosixPath('/')]
>>> list(reversed(p.parents))
[PurePosixPath('/'), PurePosixPath('/foo'), PurePosixPath('/foo/bar'), PurePosixPath('/foo/bar/baz'), PurePosixPath('/foo/bar/baz/qux')]

[vodorok]

__setattr__ is not overridden in this class. Why aren't you use the conventional way to add the root member? We are also not inheriting from a class where it is changed. I might be missing something

[vodorok]

Why not use self.options()?

[vodorok]

I am failing to comprehend the magic behind this __getattribute__

[whisperity]

It is the same thing as throughout all the configuration access classes. They are special because they provide the member access . operator's results dynamically. When you say O.foo, it runs getattr(O, "foo"). This can be overloaded by providing O.__getattr__(self, str): the interpreter will first refer the member __getattr__ function from the object's corresponding type, and then call that to perform the resolution. (This gets more involved when there are multiple non-trivial paths in the __mro__ (multiple inheritance), but this patch doesn't abuse that feature!)

Now we have a function that can return a "member" based on a string at runtime, and that thing does not even have to be a real member of the class! The problem now arises if you want to actually access "real" members that the custom __getattr__ isn't taught about. Calling object.__getattribute__ explicitly instructs the program to disregard whatever customisations of __getattr__ there might be and run the "default" member access ("attribute lookup") logic unconditionally.

[vodorok]

I can imagine an example of this validation leading to"funny-looking" options or directories.
Eg.: when there is a [] in the middle preceding the final [] like /2ddirectory[][]/ Or when using, multiple / for specifying the option, .//path/to/option//// and the root parent was passed in as a parent.

[vodorok]

Maybe add a few new testcases that verifies that only the correct options and directories can be added.

[vodorok]

Why is this function not part of the Configuration class?

[whisperity]

The only way it would make sense there as if it was a @classmethod or even better, a @staticmethod. However, this function does not consider anything related to the nice schematic access of configurations, so it would look odd to have it in there, a function that "talks" on a completely different abstraction level.

[bruntib]

This development is really impressive and comprehensive design. Maybe, slightly more than what we need. I would suggest not to change server configuration at this point as it is not unavoidable for async-store feature. Please, rebase the next 3 PRs so this server config is not the part of them. Thank you!