csingley / ofxtools

Python OFX Library
Other
301 stars 68 forks source link

Library attempts to create folders when imported #143

Closed Karlinde closed 2 years ago

Karlinde commented 2 years ago

Due to the presence of the following lines directly in the body of ofxtools/config/__init__py, this library cannot be used in a python application run by a system user without a conventional home folder:

USERCONFIGDIR.mkdir(parents=True, exist_ok=True)
...
DATADIR.mkdir(parents=True, exist_ok=True)

I encountered this when using ofxtools in a django application. It worked fine for local development, but when I deployed to a production environment the application is run by the www-data user which has a home folder of /var/www so the library tried to create the folder /var/www/.config, which isn't allowed due to the particular folder permissions used. When I tried to comment out the above lines in ofxtools/config/__init__py, the application ran as expected (though I did not test any of the ofxtools functionality).

Is it possible to move the creation of these directories into the functions where they are needed, and not have them automatically done at module import time?

csingley commented 2 years ago

Hmmm, that's not great.

Is it possible to move the creation of these directories into the functions where they are needed, and not have them automatically done at module import time?

Well I'm not sure that's the right fix. I mean, if you need to use the config module... then these directories are indeed created when they are needed. If you don't want the directories created, then just don't import the config module.

Rather, it looks to me like problem is that other modules are importing the config promiscuously.

Looks like ofxtools.Parser imports ofxtools.config just in case you want to run the parser as a script, in order to set up logging. ofxtools.Client is importing ofxtools.config simply to define a well-known location to cache PROFRS, with no way to modify that behavior. Pretty dumb.

Karlinde commented 2 years ago

I'm not that familiar with how to do conditional imports based on if running as a script or not, so I would suggest just avoiding having these two lines in the root of config/__init__.py to solve this particular problem.

I tested a fix by moving USERCONFIGDIR.mkdir(...) inside of configure_logging() since that function already runs a mkdir, and eliminated DATADIR.mkdir(...) entirely since Client.py already runs persistdir.mkdir(...) when needed. It would seem more robust to require the callers to ensure that the directories they wish to use exist, so there shouldn't be a clear need for this to be done at in __init__.py.

I removed ~/.config/ofxtools and ~/.local/share/ofxtools before running the tests. The folders were created as expected and all test cases seem to still pass. Mypy seems to complain on the typing in ofxhome.py and ofxget.py but it didn't pass even before my changes so I would guess it's not related.

Would you prefer I solve this another way or would you accept a pull request with this fix?

csingley commented 2 years ago

Fair enough, that all makes sense. I have to admit that modifying the user's filesystem at import time is kind of nasty.

Let's make it happen.