[question/feature request] Should all contexts implement a reset() method?

[margaretselzer]

When I use the same context several times to seralize different expressions, e.g.

ConnT::_serializer_context_t ctx;
str1 = sqlpp::serialize(expression1, ctx).str();
str2 = sqlpp::serialize(expression2, ctx).str();

Then str2 will actually be holding str1 + str2.

This is because the underlying stream buffer is never reset. Hence the suggestion to implement reset() which in turn could call _os.str("") the same way as it is done for MockDb::_serializer_context_t. (Side question: Why does the mock context have features that the real contexts do not?)

Disclaimer: I need this way of using a serializer to dynamically build a where clause from sqlpp expressions, so when the new dynamic() function makes it to the main branch I will not need this any more. Nevertheless, it might not be a bad thing to add it ... at least making the real databases and the mock have the same interfaces.

WDYT?

[MeanSquaredError]

Side question: Why does the mock context have features that the real contexts do not?

I am not familiar with all the gory details of the contexts, but most likely the mock context just went out of sync with the real contexts. I remember that when I was trying to unify the APIs provided by the three connectors, I updated the actual connectors' code and the sample code in connector_api/, but I did not change the MockDB code at that time because it was working fine the way it was. I thought that I will change it later in another PR but was sidetracked by other issues. So I suspect that something similar happened with the mock context too even though my PRs did not change the real or mock contexts.

EDIT: Initially the connectors were separate (github) projects from the core sqlpp11 project, so their APIs varied quite a bit. So I guess this could be yet another reason why the APIs of the mock context and the real contexts diverged quite easily.

[margaretselzer]

Yepp, these kind of things easily happen. ;-) The question is if it should be fixed or not and how. Add reset() to the APIs or remove it from the mock. My 5 cent is on adding reset() to the APIs...

[rbock]

Thanks for the discussion! I guess you will be happy to learn that the serialize function has been replaced by to_sql_string in the optional-no-dynamic branch :-)

Your example would look like this:

ConnT::_serializer_context_t ctx;
str1 = sqlpp::to_sql_string(ctx, expression1);
str2 = sqlpp::to_sql_string(ctx, expression2);

It would just work[*].

For now, resetting the context as part of calling str sounds reasonable to me.

Cheers,
Roland

[*] Unless you expressions contain parameters. ctx maintains the current index, i.e. the number of parameters in expression1 influences the string generated from expression2. But this would also cause issues while executing the statement. So I don't think this would be relevant.

[margaretselzer]

For now, resetting the context as part of calling str sounds reasonable to me.

Do you actually mean modifying the str() functions to:

      std::string str() const
      {
        _os.str("");  // Reset the stream buffer
        return _os.str();
      }

?

[rbock]

That particular implementation would yield an empty string, I think? But yes, modifying the str function.

WDYT?

[margaretselzer]

That particular implementation would yield an empty string, I think? But yes, modifying the str function.

The greatness in this solution is that it surely will not contain results of previous calls. :)

Yeah, without thinking I just put there this buffer reset which should be called before serialize is called. But then I do not understand how you mean to modify str(). This function can only manipulate the underlying stream and it will not know when and how many times serialize() was called.

And just to be sure that we are talking about the same thing, I mean std::string context_t::str() const. (Now that I see that it is const, my resetting the buffer there would not even compile. :) )

[rbock]

Oops, yeah, it is const. We should not change that. Otherwise we could have done something like

std::string str() /* if it were not const */
{
   auto buf = _os.str();
   _os.str("");  // Reset the stream buffer
    return buf;
}

Please ignore my suggestion :-)

[margaretselzer]

if it were not const

It would feel really aggravating that I did not see your solution. :-[
Luckily I can just ignore it now! :)

But then what do we do? reset() as in MockDb or leave it be until ...no-dynamic reaches the shores?

[rbock]

If you can cope for now, e.g. by using a new context each time you serialize something manually, I suggest we wait.

[margaretselzer]

Sure, I have a solution. It's just for the library as a whole. If you can address syncing the interfaces and mock in optional-no-dynamic then no reason to change it now.