I know I'm late to this discussion. My last week has been hectic.
On Sun, Nov 24, 2019 at 03:37:23PM +0000, Gary Buhrmaster wrote:
> On Sat, Nov 2, 2019 at 4:23 PM Klaas de Waal <klaas.de.waal@gmail.com> wrote:
>
> > I have now added the support for the Services API along the same lines
> > as previously done for the bouquet_id and the region_id. It is
> > convenient to assume that the version bump done by Bill yesterday does
> > also cover this change.
>
> That change is necessary, as It provides the
> appropriate ability for a user (app) of the service
> API to identify when enable the new features.
>
> But that does not provide API backward
> compatibility when using an existing endpoint
> where existing codes which do not provide the
> values do not get existing values overridden by
> updates that do not provide them (likely using
> something like the checks that were mentioned
> by Roger to check if the names were supplied
> in the post, and if not, do not update those
> values in a row, or ensuring that existing values
> are maintained (depending on how you wish
> to write the code).
I know the API layer allows for version numbers for each entry point.
Does anyone know if it also does for the abstract types too (i.e
VideoSource, Channel, etc.)? IMHO, for each entry point, we should
strive to support at least two versions -- the one for the current,
MythTV release and the one for the previous, MythTV release.
Does anyone know if (and how) the API layer allows for multiple
versions of the same entry points to be supported simultaneously?
I've not seen how to do it. If we don't have that capability, I
suggest we brute force it by appending the MythTV, major, version
number to anything that changes. For example, with the recent
buquet_id and region_id changes, any types and methods that were
changed would use a new, "31" suffix.
Note that I know virtually nothing of the API, layer internals and
maybe my suffix suggestion is too naive. The intent to support the
current and previous, API versions remains.
On Tue, Nov 26, 2019 at 01:43:27AM +0000, Gary Buhrmaster wrote:
> On Mon, Nov 25, 2019 at 6:22 AM Bill Meek <keemllib@gmail.com> wrote:
>
> > I *think* the attached is what Gary/Roger are suggesting. But when I
> > 'signed on', a senior developer said there shouldn't be any SQL in the
> > API code (but there was already.)
>
> (parenthetical comment: even senior devs can find
> that they have to make compromises in the real
> world unless they are one of the few with infinite
> time to refactor all the codes).
>
> That is one approach, another is to test for whether
> the value was provided (as Roger demonstrated in
> his reference), and, if not, do not update the columns
> (using conditional creation of the update SQL to not
> update columns not provided). Pseudo code follows.
>
> And so can creating an entirely new endpoint,
> although in this case I think that is overkill.
>
> Any of those can work, so it would depend on what
> makes sense as example going forward for other
> new devs who are not otherwise familiar with their
> requirements to maintain API compatibility (i.e.
> the added code, whatever it will be, will typically
> be copied for the next case).
>
> As this is a question for the devs (they will have
> to live with the solution going forward), I will defer
> to them to decide which is the best way to propose
> to fix the API breakage.
>
> pseudo code:
> // existing code (before new columns)
> QString updateSQL
> MSqlBindings bindings;
>
> updateSQL = "Update table .... set column1 = :Column1, column2 = :Column2"
> bindings[":Column1"] = Value1
> bindings[":Column2"] = Value2
>
> // Added code
> if (m_parsedParams.contains("RegionId"))
> updateSQL += " set regionid = :RegionId ";
> bindings[":RegionId"] = RegionId;
I like the idea of no SQL in the intermediate, API layers. It wasn't
me who suggested it earlier, though. Also, I don't think the update
only the chagned values will work in the general case. It might not
be possible to validate the changed values without knowing some of the
unchanged values too. I think the SQL read, merge changes and update
approach is best and should be done in the lowest, API layer. As has
been noted, we need some way of knowing what values are provided (and
which ones aren't) in the current invocation.
On Mon, Nov 25, 2019 at 08:00:32PM -0600, Bill Meek wrote:
> On 11/25/19 7:43 PM, Gary Buhrmaster wrote:
> > On Mon, Nov 25, 2019 at 6:22 AM Bill Meek <keemllib@gmail.com> wrote:
> >
> > > I *think* the attached is what Gary/Roger are suggesting. But when I
> > > 'signed on', a senior developer said there shouldn't be any SQL in the
> > > API code (but there was already.)
> >
> > (parenthetical comment: even senior devs can find
> > that they have to make compromises in the real
> > world unless they are one of the few with infinite
> > time to refactor all the codes).
> >
> > That is one approach, another is to test for whether
> > the value was provided (as Roger demonstrated in
> > his reference), and, if not, do not update the columns
> > (using conditional creation of the update SQL to not
> > update columns not provided). Pseudo code follows.
> >
> > And so can creating an entirely new endpoint,
> > although in this case I think that is overkill.
> >
> > Any of those can work, so it would depend on what
> > makes sense as example going forward for other
> > new devs who are not otherwise familiar with their
> > requirements to maintain API compatibility (i.e.
> > the added code, whatever it will be, will typically
> > be copied for the next case).
> >
> > As this is a question for the devs (they will have
> > to live with the solution going forward), I will defer
> > to them to decide which is the best way to propose
> > to fix the API breakage.
> >
> > pseudo code:
> > // existing code (before new columns)
> > QString updateSQL
> > MSqlBindings bindings;
> >
> > updateSQL = "Update table .... set column1 = :Column1, column2 = :Column2"
> > bindings[":Column1"] = Value1
> > bindings[":Column2"] = Value2
> >
> > // Added code
> > if (m_parsedParams.contains("RegionId"))
> > updateSQL += " set regionid = :RegionId ";
> > bindings[":RegionId"] = RegionId;
>
> Right, I'm using the m_parsedParms... solution. And in line SQL to get the existing values
> and the existing call to SourceUtil::UpdateSource(). Testing now.
What is this m_parsedParms? I seem to have missed it. Is it
something that states which paramerters are provided in an API
invocation?
Regarding knowing which parameters are provided, I'd previously
suggested using QVariants for everything. That would be messy, so I'm
now thinking a better idea might be to have the API layer pass a
QSet<QString> to each entry point with names of all provided
paramaters.
On Tue, Nov 26, 2019 at 02:31:12AM +0000, Gary Buhrmaster wrote:
> On Tue, Nov 26, 2019 at 2:01 AM Bill Meek <keemllib@gmail.com> wrote:
>
> > Fun fact: this endpoint broke mythfilldatabase if a user had a grabber
> > because the configpath gets set to "" rather than NULL. mythfilldatabase
> > depends on the NULL case to develop the path to the file automatically.
> > If ConfigPath=someValidPath, then it's OK.
>
> As I mentioned previously, there is a number
> of cases where NULL does not equal empty
> throughout the code base (sometimes for
> good reasons, some times I am not so sure),
> and the Services API generally does not
> provide a way to set a NULL and/or an
> empty value. Realistically there are only a
> few apps today that try to implement an
> alternative to mythtv-setup, so the impact is
> limited, but as I understand it the stated goal
> was to move towards a web based setup
> (using the services API), which makes the
> side cases more interesting to address.
I'm now thinking that in cases where NULL is ambiguous with
empty/0/whatever, we could use the convetion of having value or
valueIsNull paramerters where one or the other, but not both, is
present.
David
--
David Engel
david@istwok.net
_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev http://wiki.mythtv.org/Mailing_List_etiquette MythTV Forums:
https://forum.mythtv.org
