openSUSE / open-build-service

Build and distribute Linux packages from sources in an automatic, consistent and reproducible way #obs
https://openbuildservice.org
GNU General Public License v2.0
931 stars 438 forks source link

Inconsistent behavior in API when listing packages in project #9715

Open crazyscientist opened 4 years ago

crazyscientist commented 4 years ago

Issue Description

During development I like looking at the raw XML of API responses and I came this inconsistency in the API route GET /source/<project>:

The API docs say that the deleted parameter can be used to "show deleted package instead of existing" ones. That lead me to the believe that deleted=0 would be the default and could be omitted.

Now imagine my surprise that these queries return different results:

The first query only returns ordinary package names and a count:

<directory count="12987">
  <entry name="000package-groups"/>
  <entry name="000product"/>
  <entry name="000release-packages"/>
  <entry name="000update-repos"/>
  ...
</directory>

, whereas the second one includes multibuild flavored packages, but no count:

<directory>
  ...
  <entry name="000release-packages"/>
  <entry name="000release-packages:openSUSE-Addon-NonOss-release" originpackage="000release-packages"/>
  <entry name="000release-packages:openSUSE-MicroOS-release" originpackage="000release-packages"/>
  <entry name="000release-packages:openSUSE-release" originpackage="000release-packages"/>
  ...
</directory>

Expected Result

I agree with the docs that deleted=0 should be assumed as the default.

However omitting the deleted parameter in the query should not affect the API response in this way.

How to Reproduce

  1. Save query responses
  2. diff the responses
adrianschroeter commented 2 years ago

this is a mismatch of backend and api answers. The backend maybe should not list flavors. However, where is the problem?

crazyscientist commented 2 years ago

However, where is the problem?

OK, call me pedantic, but if the docs say the boolean parameter deleted has a default value, I expect that I can omit the parameter and receive the identical response. But this s not the case:

dmach commented 2 years ago

I also think that using deleted option to control if flavors of multibuild packages are listed is confusing at least. In my opinion the behavior of deleted=0 and omitted deleted option should be unified. If flavors shouldn't be part of the default output, we might need another option for them.

adrianschroeter commented 2 years ago

The =0 and =1 is anyway just a workaround of a limitation in rails cgi parameter handling, it should be only &deleted actually, from my side I consider deleted=0 as something which should actually not be used. One could of course add code at plenty places in the api to handle =0 special by removing the parameter entirely, but I am not really convinced that this is worth the effort though.

hennevogel commented 2 years ago

The =0 and =1 is anyway just a workaround of a limitation in rails cgi parameter handling

The controller just checks for the existence of the parameter and passes it on.

    if params.key?(:deleted)
      ...
      pass_to_backend
      return
    end

So it will also pass on ?deleted= or ?deleted

crazyscientist commented 2 years ago

The =0 and =1 is anyway just a workaround of a limitation in rails cgi parameter handling, it should be only &deleted actually

Again, call me pedantic. I'm aware that GET query strings can have an arbitrary format, but given that my only resources are the API docs and the sources of osc [^1], how should I know that I'm supposed to use this parameter as a flag without value?

Except for OBS, I'm not aware of a single case where the convention of using name=value pairs is violated.

So, as somebody who is used to virtually every system following the convention, I would consider it good practice if these queries produced identical output:

I'd like to remind you, these queries return a list of the same packages, only the amount of information is different. And that difference is bothering me. As @dmach pointed out, it is terribly confusing to use the deleted=0 query to add variants to the result.

On the other hand, ?deleted and ?deleted=1 produce the identical output as I would expect.

[^1]: Which makes heavy use of the =x format, by the way.

adrianschroeter commented 2 years ago

On Freitag, 25. Februar 2022, 13:21:32 CET Henne Vogelsang wrote:

The =0 and =1 is anyway just a workaround of a limitation in rails cgi parameter handling

The controller just checks for the existence of the parameter and passes it on.

    if params.key?(:deleted)
      ...
      pass_to_backend
      return
    end

So it will also pass on ?deleted= or ?deleted

might work meanwhile (or only here), but it used not to be and therefore we had to introduce the =X workaround (not limited to deleted, but in general). And now it is unfortunatly part of the documented api.

In this case might want to update the documentation that the parameter alone is enough. The =X ways must still work of course.

However, this is independend of the inconsistency, we discussed it and will most likely change the backend that the source server is not reporting the flavors anymore by default. But adding a parameter to list them again.

--

Adrian Schroeter @.***> Build Infrastructure Project Manager

SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev

crazyscientist commented 2 years ago

The =0 and =1 is anyway just a workaround of a limitation in rails cgi parameter handling

If I'm not mistaken, many high-level frameworks and languages, do not follow strictly the standard definition, which allows flags (i.e. parameters without value), but rather the mantra "explicit is better than implicit".

So, if your framework and many others are not favoring flags, maybe should abandon the idea of flags?

hennevogel commented 2 years ago

As I showed in the code it has nothing to do with any framework and just with the way it's implemented :)

adrianschroeter commented 2 years ago

the docu is stating already that deleted is a boolean. The =somehting is just there for backward compability, but should not be used anymore.

crazyscientist commented 2 years ago

the docu is stating already that deleted is a boolean.

I learned that a boolean is either 0 or 1. And the API did and does not processes boolean values correctly.

Instead of a Boolean, there is null or undefined (false) or "something" (true) expected. And in case of something the behavior depends on the actual value of "something".

So, while I would have considered this issue to be a bug, I certainly don't see that the documentation describes the behavior of the API accurately.

crazyscientist commented 2 years ago

the docu is stating already that deleted is a boolean.

@adrianschroeter for some endpoints it does, but for others it does not, e.g. (screenshot taken just now on OBS):

image

According to the docs, deleted is no boolean param for this endpoint. BTW, I came across a few endpoints, where ?foo is not understood, but ?foo=[01] is the only option.