Adding altered objects to sessions without committing

[mircealungu]

A method that alters objects that are mapped via SQLAlchemy to the DB should append the objects to the session, but not commit the session. Instead the commit should be done by the method that has obtained independently the session and has passed it down to to the other methods.

Correct:

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_map = UserCohortMap(user=self, cohort=cohort)
        session.add(new_map)

Incorrect:

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_map = UserCohortMap(user=self, cohort=cohort)
        session.add(new_map)
        session.commit()

[mircealungu]

Fragment from a conversation that lead to this decision:

When we add/remove a user from a cohort, should we commit immediately or add it to the session and then the developer can decide when to commit the change?

are you talking about this? https://github.com/zeeguu/api/pull/206/files#diff-bb192734c50e63f5c18ff35dcab929883ceadefef56fa5c32fef107d21bf7d2eR321
It s ok how you're doing it. Although every commit takes a bit of time.
We could try to create a rule of thumb that says that something like:

things should be committed by the caller if possible

or

if a method gets a session, it should modify it, but not commit by default

So in this case then the add_user_to_cohort would just add the user to the session but leave the commit to be done by the parent.
However, I'm not sure that there won't be exceptions from these rules.

Yes, it's just that in the code you can see in the addition I do the commit in the method in add user, while in the delete I am doing it in the API call rather than the User.Method, where it just adds the value to be deleted to the session, it can then be deleted at the API level. I think that it creates a bit of a conflict in the semantics of add/delete user, if the user is not really added or deleted. What do you think we commit, and if we decide to use sessions, maybe we should call the method "mark_for_delete/add" - so it's more explicit it doesn't delete the user right away.

this is an interesting idea, but i wonder if it won't make our method names too long?

    def mark_for_add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_cohort = UserCohortMap(user=self, cohort=cohort)
        session.add(new_cohort)

the more we discuss the more I think that somebody who gets a session should not commit it.
you get a session you add to it, and that's it.

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_cohort = UserCohortMap(user=self, cohort=cohort)
        session.add(new_cohort)

alternative would be to not pass the session at all anymore, but instead such methods that aim to modify the DB model always return the objects that have to be added to the session and committed?

e.g.

    def add_user_to_cohort(self, cohort, session):
        from zeeguu.core.model.user_cohort_map import UserCohortMap

        new_user_cohort_map = UserCohortMap(user=self, cohort=cohort)
        return new_user_cohort_map

then the caller, probably the api endpoint who already has a session handy does

    new_mapping = flask.g.user.add_user_to_cohort(cohort)
    session.add(new_mapping)
    session.commit()

do you see what I mean?

surely, this might become complicated, because when you want to add multiple objects then you have to have to create a temporary array to which you add all these objects, and then you return it. So probably the solution above with passing the session and adding directly to it is still the most concise. And functionally it's exactly the same as the one with returning the objects to be added. However, in a sense it's more clear that things have to be added to the session.