Thread: xpath changes in the recent back branches
Hi,
Commit 79af9a1d2668c9edc8171f03c39e7fed571eeb98 changed xpath handling
with regard to namespaces, and it seems to be fixing an actual issue.
However, it was also backpatched to all branches despite it breaking for
example code like this:
do $$
declare
_x xml;
begin
_x := (xpath('/x:Foo/x:Bar', xml '<Foo
xmlns="teh:urn"><Bar><Baz>1</Baz><Bat>2</Bat></Bar></Foo>',
array[['x','teh:urn']]))[1];
raise notice '%', xpath('/Bar/Baz/text()', _x);
raise notice '%', xpath('/Bar/Bat/text()', _x);
end
$$;
The problem is that there's no way to write the code like this in such a
way that it would work on both versions. If I add the namespace, it's
broken on 9.1.14. Without it it's broken on 9.1.15.
I'm now thinking of adding a workaround which strips namespaces, but
that doesn't seem to be easy to do, even with PL/Perl. Is there a
better workaround here that I'm not seeing?
I'm not sure how changing behavior like this in a minor release was
considered acceptable.
.m
On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja <marko@joh.to> wrote:
> Commit 79af9a1d2668c9edc8171f03c39e7fed571eeb98 changed xpath handling with
> regard to namespaces, and it seems to be fixing an actual issue. However, it
> was also backpatched to all branches despite it breaking for example code
> like this:
>
> do $$
> declare
> _x xml;
> begin
> _x := (xpath('/x:Foo/x:Bar', xml '<Foo
> xmlns="teh:urn"><Bar><Baz>1</Baz><Bat>2</Bat></Bar></Foo>',
> array[['x','teh:urn']]))[1];
> raise notice '%', xpath('/Bar/Baz/text()', _x);
> raise notice '%', xpath('/Bar/Bat/text()', _x);
> end
> $$;
>
> The problem is that there's no way to write the code like this in such a way
> that it would work on both versions. If I add the namespace, it's broken on
> 9.1.14. Without it it's broken on 9.1.15.
That certainly sucks.
> I'm not sure how changing behavior like this in a minor release was
> considered acceptable.
I'm guessing that the fact that it changes behavior in cases like this
wasn't recognized, but I suppose Peter will have to be the one to
comment on that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja <marko@joh.to> wrote:
>> I'm not sure how changing behavior like this in a minor release was
>> considered acceptable.
> I'm guessing that the fact that it changes behavior in cases like this
> wasn't recognized, but I suppose Peter will have to be the one to
> comment on that.
It was considered to be a bug fix; more, given the few complaints about
the clearly-broken old behavior, we thought it was a fix that would affect
few people, and them positively. Sorry you're not happy about it, but
these things are always judgment calls.
regards, tom lane
On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja <marko@joh.to> wrote:
Hi,
Commit 79af9a1d2668c9edc8171f03c39e7fed571eeb98 changed xpath handling with regard to namespaces, and it seems to be fixing an actual issue. However, it was also backpatched to all branches despite it breaking for example code like this:
do $$
declare
_x xml;
begin
_x := (xpath('/x:Foo/x:Bar', xml '<Foo xmlns="teh:urn"><Bar><Baz>1</Baz><Bat>2</Bat></Bar></Foo>', array[['x','teh:urn']]))[1];
raise notice '%', xpath('/Bar/Baz/text()', _x);
raise notice '%', xpath('/Bar/Bat/text()', _x);
end
$$;
The problem is that there's no way to write the code like this in such a way that it would work on both versions. If I add the namespace, it's broken on 9.1.14. Without it it's broken on 9.1.15.
I'm now thinking of adding a workaround which strips namespaces, but that doesn't seem to be easy to do, even with PL/Perl. Is there a better workaround here that I'm not seeing?
FWIW, I've been working around the bug fixed in that commit for ages by spelling my xpath like this:
xpath('/*[local-name()="Bar"]/*[local-name()="Baz"]/text()', data)
I've modularized my XML handling functions so the source of 'data' is immaterial -- maybe it's a full document, maybe it's a fragment from a previous xpath() call -- and the referenced commit is going to make correct XPATH much more sane, readable, and maintainable. I, for one, welcome it wholeheartedly.
HTH,
--Mike
On 3/4/15 5:26 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Feb 19, 2015 at 5:53 AM, Marko Tiikkaja <marko@joh.to> wrote: >>> I'm not sure how changing behavior like this in a minor release was >>> considered acceptable. > >> I'm guessing that the fact that it changes behavior in cases like this >> wasn't recognized, but I suppose Peter will have to be the one to >> comment on that. > > It was considered to be a bug fix; more, given the few complaints about > the clearly-broken old behavior, we thought it was a fix that would affect > few people, and them positively. Yeah, but these things usually go the other way. "This has been broken for 10 years but nobody noticed, so we're not going to fix this" is the answer I'm more used to seeing. And frankly, even though I remember getting the wrong end of that hose a few times, I prefer that to breaking apps in minor releases. .m
On 3/4/15 6:19 PM, I wrote: > On 3/4/15 5:26 PM, Tom Lane wrote: >> It was considered to be a bug fix; more, given the few complaints about >> the clearly-broken old behavior, we thought it was a fix that would affect >> few people, and them positively. > > Yeah, but these things usually go the other way. "This has been broken > for 10 years but nobody noticed, so we're not going to fix this" Sorry, that should have said "we're not going to fix this *in the back branches*". .m
On 3/4/15 12:20 PM, Marko Tiikkaja wrote: > On 3/4/15 6:19 PM, I wrote: >> On 3/4/15 5:26 PM, Tom Lane wrote: >>> It was considered to be a bug fix; more, given the few complaints about >>> the clearly-broken old behavior, we thought it was a fix that would >>> affect >>> few people, and them positively. >> >> Yeah, but these things usually go the other way. "This has been broken >> for 10 years but nobody noticed, so we're not going to fix this" > > Sorry, that should have said "we're not going to fix this *in the back > branches*". I tend to agree. I was not in favor of backpatching, but other people were in favor of it, and no one spoke up against it. That said, if I understand your test case correctly, this would basically be an argument against any semantic corrections in stable releases, since user code could expect to continue to work with the previous incorrect value. You can always test the server version number, and you'll have to for the next major release anyway.
On Wed, Mar 4, 2015 at 4:11 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > That said, if I understand your test case correctly, this would > basically be an argument against any semantic corrections in stable > releases, since user code could expect to continue to work with the > previous incorrect value. > > You can always test the server version number, and you'll have to for > the next major release anyway. That's not really the problem, of course. The problem is you upgrade and your app unexpectedly breaks. The complexity of fixing that once it's happened is not entirely irrelevant, but it's not really the main problem, either. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 3/4/15 5:00 PM, Robert Haas wrote: >> You can always test the server version number, and you'll have to for >> the next major release anyway. > > That's not really the problem, of course. The problem is you upgrade > and your app unexpectedly breaks. The complexity of fixing that once > it's happened is not entirely irrelevant, but it's not really the main > problem, either. That paragraph was mainly a response to the claim that there is "no way" to make it work in both old and new versions.