936275 - In buglists, flags are sorted alphabetically instead of using their sortkey

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Simon Green]

Attachment: v1 patch

When viewing bug lists, the flags are sorted alphabetically for a bug rather than by their sort order (flagtypes.sortkey, flagtypes.name)

Comment 1

[Simon Green]

Comment on attachment 828977 [details] [diff] [review]
v1 patch

This only works for MySQL and Postgres >= 9.

If someone wants to make a patch for the other databases, feel free too. As it stands, the other databases will still sort alphabetically.

It also contains some CC SA 3.0 code, as noted in the comments in Bugzilla/DB/Pg.pm. IANAL, so am unsure if this is compatible with MPL 2.0, but assume it is.

Comment 2

[Simon Green]

(relnote for the fact that sql_group_contact allows an order by value for extension code, not for the fact that flags are reordered)

Comment 3

[Frédéric Buclin]

Comment on attachment 828977 [details] [diff] [review]
v1 patch

>=== modified file 'Bugzilla/DB/Pg.pm'

>+            # Since Postgres (quite rightly) doesn't support "SELECT DISINCT x

s/DISINCT/DISTINCT/


>+            return "ARRAY_TO_STRING(ANYARRAY_UNIQ(ARRAY_AGG($1 ORDER BY $order_by)), $separator)";

What's the problem with Pg 8.x? What is not supported?


>+    if ($self->get_server_version > 9000) {

Above, you wrote >= 9000. Here, you write > 9000. Which one is correct?


>+        # anyarray by Joshua D. Burns is licensed under the Creative Commons
>+        # Attribution 3.0 Unported License https://github.com/JDBurnZ/anyarray

IMO, it's not OK to mix licenses. Bugzilla license is MPL 2.0.

Comment 4

[Frédéric Buclin]

@gerv: could you answer the legal question from my previous comment?

Comment 5

[Frédéric Buclin]

Comment on attachment 828977 [details] [diff] [review]
v1 patch

>+sub get_server_version {
>+    return Bugzilla->dbh->{pg_server_version};
>+}

We already have $dbh->bz_server_version which returns this information. We don't need a separate method.

Comment 6

[Frédéric Buclin]

This seems a bit invasive for 4.4. And the current behavior is like this since it has first been implemented in 3.6.

No need to relnote it. We don't relnote when we change internal functions. And sql_group_concat() has never been relnoted, even when it has been implemented.

Comment 7

[Simon Green]

(In reply to Frédéric Buclin from comment #3)
> Comment on attachment 828977 [details] [diff] [review]
> v1 patch
> 
> >=== modified file 'Bugzilla/DB/Pg.pm'
> 
> >+            # Since Postgres (quite rightly) doesn't support "SELECT DISINCT x
> 
> s/DISINCT/DISTINCT/
> 
> 
> >+            return "ARRAY_TO_STRING(ANYARRAY_UNIQ(ARRAY_AGG($1 ORDER BY $order_by)), $separator)";
> 
> What's the problem with Pg 8.x? What is not supported?

I thought it doesn't have all the functions like ARRAY_TO_STRING and ARRAY_AGG, but checking the PostgreSQL documentation, it seems it does. I only have a PostgreSQL 9.2 server to test this with, but I trust their documentation.

However, PostgreSQL 7.4 doesn't have ARRAY_AGG. Postgres 7.4 is EOL, so if we make the minimum v8, I won't need to worry about supporting 7.4. Considering Bugzilla 5 is still many months away, I think this is fair enough. Any objections?

> >+        # anyarray by Joshua D. Burns is licensed under the Creative Commons
> >+        # Attribution 3.0 Unported License https://github.com/JDBurnZ/anyarray
> 
> IMO, it's not OK to mix licenses. Bugzilla license is MPL 2.0.

The code including the change will still be 100% MPL 2.0 licensed. My question is more around is it okay to take CC SA code and put it in MPL code. From what I read, I think it is okay (providing we give attribution), but IANAL.

(In reply to Frédéric Buclin from comment #6)
> This seems a bit invasive for 4.4. And the current behavior is like this
> since it has first been implemented in 3.6.

This was always intended for upstream Bugzilla 5.0. BRC will back port it to 4.4, and I imagine BMO might even take it for 4.2.

> No need to relnote it. We don't relnote when we change internal functions.
> And sql_group_concat() has never been relnoted, even when it has been
> implemented.

Noted.

I'll submit a new patch on Monday, assuming it is okay to ditch support for PostgreSQL 7.4

Comment 8

[Frédéric Buclin]

(In reply to Simon Green from comment #7)
> However, PostgreSQL 7.4 doesn't have ARRAY_AGG. Postgres 7.4 is EOL, so if
> we make the minimum v8, I won't need to worry about supporting 7.4.
> Considering Bugzilla 5 is still many months away, I think this is fair
> enough. Any objections?

FYI, we require Pg 8.3 since Bugzilla 4.2. You are a bit behind with requirements. :-D

Comment 9

[Simon Green]

Attachment: v2 patch

Made suggested changes. It turns out STRING_AGG isn't supported in Pg 8, but I managed to work around this.

Comment 10

[Gervase Markham [:gerv]]

The MPL and CC-BY 3.0 can be mixed in a project, as long as the code is in distinct files. Whether we want to do this as a matter of policy is one for justdave, although we do have precedent in that we have 3rd party libraries under other permissive code licenses, such as MIT. CC is a bad license for software - even the CC people say that. Someone could send him a polite note to that effect :-)

Gerv

Comment 11

[Frédéric Buclin]

Comment on attachment 829982 [details] [diff] [review]
v2 patch

I honestly don't plan to review this. Code changes in Bugzilla/DB/Pg.pm are definitely out of my skills; I know nothing about plpgsql (and it uses a different license). It's a heavy change about Pg functions used just for flags. And https://github.com/JDBurnZ/anyarray/blob/master/README.md says that this code hasn't been tested with Pg 8.3.

Retargetting to glob.

Comment 12

[:glob ✱]

Comment on attachment 829982 [details] [diff] [review]
v2 patch

i too know nothing about pg - retargetting to 'anyone'.

Comment 13

[Simon Green]

Attachment: bug936275-v3.patch

dylan: You were nominated by your boss since you appear to be the other Pg expert. Once you have approved the patch, it will go to another reviewer for their review too.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8440271 [details] [diff] [review]
bug936275-v3.patch

Review of attachment 8440271 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, this works on my system and the code seems fine. I'm a little worried about the performance of the array functions, but I can't see a better way of accomplishing this.

::: Bugzilla/DB/Pg.pm
@@ +84,4 @@
>      $sort = 1 if !defined $sort;
>      $separator = $self->quote(', ') if !defined $separator;
> +
> +    if ($order_by && $text =~ /DISTINCT\s*(.+)$/) {

Shouldn't this be /^DISTINCT.../ in addition the terminal anchor $?

@@ +258,5 @@
> +            RETURN return_array;
> +        END;
> +        $BODY$ LANGUAGE plpgsql;
> +        |
> +    );

nit: I believe this SQL should be on the same line as the closing quote |,
as in |); (at the same indent level as the );)

@@ +259,5 @@
> +        END;
> +        $BODY$ LANGUAGE plpgsql;
> +        |
> +    );
> +

nit: extra empty line

Comment 15

[:glob ✱]

Comment on attachment 8440271 [details] [diff] [review]
bug936275-v3.patch

Review of attachment 8440271 [details] [diff] [review]:
-----------------------------------------------------------------

due to the inverted vers_cmp breaking pg 8, let's get an updated patch which addresses that as well as the nits.

::: Bugzilla/DB/Pg.pm
@@ +84,4 @@
>      $sort = 1 if !defined $sort;
>      $separator = $self->quote(', ') if !defined $separator;
> +
> +    if ($order_by && $text =~ /DISTINCT\s*(.+)$/) {

you should ignore case on this regex (to mirror the regex at line 100)

@@ +101,5 @@
> +        $order_by = " ORDER BY $1";
> +    }
> +
> +    if (vers_cmp($self->bz_server_version, 9) > 0) {
> +        # PostgreSQL 8.x doesn't support STRING_AGG

the vers_cmp comparison is backwards.

Comment 16

[:glob ✱]

i also realised that the code don't comply with the licensing requirements as per comment 10:

(Gervase Markham [:gerv] from comment #10)
> The MPL and CC-BY 3.0 can be mixed in a project, as long as the code is in
> distinct files.

the CC licensed code isn't in a distinct file.

Comment 17

[Gervase Markham [:gerv]]

Should I ping jdburnz to ask him if it's OK to use the code under MPL 2? Happy to do so, if that's the easiest thing.

Gerv

Comment 18

[Simon Green]

(In reply to Gervase Markham [:gerv] from comment #17)
> Should I ping jdburnz to ask him if it's OK to use the code under MPL 2?
> Happy to do so, if that's the easiest thing.

It would be awesome if you can do that gerv. Would make life very easy for us.

  -- simon

Comment 19

[Gervase Markham [:gerv]]

The URL given to anyarray - https://github.com/JDBurnZ/anyarray - shows that it's under the MIT license:
https://github.com/JDBurnZ/anyarray/blob/master/LICENSE.md

So if you update the comment and paste a copy of that MIT license underneath, just above the code you've borrowed, that should do the trick.

Gerv

Comment 20

[Simon Green]

Attachment: bug936275-v4.patch

Comment 21

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8455129 [details] [diff] [review]
bug936275-v4.patch

Review of attachment 8455129 [details] [diff] [review]:
-----------------------------------------------------------------

Glad we got the license sorted out.

Comment 22

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   551eb6d..28bcce5  master -> master