maarten-dp / flask-restless-datamodel

Server side code for flask-restless-client
MIT License
1 stars 4 forks source link

Make a separate blueprint for rpc calls #9

Closed JesseDeLoore closed 10 months ago

JesseDeLoore commented 10 months ago

A proposal to make a separate blueprint for RPC calls. This allows us to not having to call register on the same blueprint multiple times. I couldn't get the functionality to not auto commit working within the test suite, I suppose there is some changed default behaviour in SQLAlchemy or Flask-SQLAlchemy that is causing this. My knowledge does not run deep enough unfortunately.

I also haven't found a way to automatically register the rpc blueprint after all the models are added. There is a possibility to use some sort of context manager, but it will require an adaption of the implementing party anyway. It probably doesn't hurt to be explicit about and give the option to not register the rpc (and just have a CRUD framework)

maarten-dp commented 10 months ago

Thanks for your pull request! It is very much appreciated!

I also haven't found a way to automatically register the rpc blueprint after all the models are added

Since this library is already a textbook example of monkeypatch madness, we could monkeypatch the app context push, or the flask run function to register the blueprint right before we enter the app context. But I'm in favor the current implementation, as it makes things, like you said, more explicit. It also has the benefit of making the rpc part "opt-in". So, to me, the current solution is more than fine.

I couldn't get the functionality to not auto commit working within the test suite

Weird, cause when I use the same env on the current master, all tests pass, but indeed, one test fails on yours. I'm unsure where this behaviour comes from, but it must have to do with moving the rpc functionality to a new blueprint.

Anyway, if you apply the following diff, it'll fix the broken test

diff --git a/flask_restless_datamodel/helpers.py b/flask_restless_datamodel/helpers.py
index 7f57da3..a9a4900 100644
--- a/flask_restless_datamodel/helpers.py
+++ b/flask_restless_datamodel/helpers.py
@@ -51,6 +51,9 @@ def run_object_method(instid, function_name, model, commit_on_return):
             session.commit()
         except Exception:
             pass
+    else:
+        session = Session.object_session(instance)
+        session.rollback()

     return result

This does undo all uncommitted changes before returning, but this should be in line with the wanted behaviour.

Anyway, if you apply the above (or an equivalent) to your branch and update the PR, I'll merge your solution and make a new release.

Thanks once more for your PR! :)

JesseDeLoore commented 10 months ago

I added the fix for the rollback. Do I need to prepare anything for a release (version, changelog, ...) or is it good as is?

JesseDeLoore commented 10 months ago

@maarten-dp if you are ok with the changes, can you merge/release ?

maarten-dp commented 10 months ago

Yes. I'll merge now and make a release this evening (or tomorrow at the latest), as I don't have access to my leasure laptop at the moment. Busy week, sorry :)

maarten-dp commented 10 months ago

By the way, thank you for your efforts!

maarten-dp commented 10 months ago

0.3.0 is released, let me know if you run into issues.