Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behavior in API when listing packages in project #9715

Open
crazyscientist opened this issue Jun 10, 2020 · 12 comments
Open

Inconsistent behavior in API when listing packages in project #9715

crazyscientist opened this issue Jun 10, 2020 · 12 comments
Labels
Frontend Things related to the OBS RoR app

Comments

@crazyscientist
Copy link

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
Copy link
Member

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

@crazyscientist
Copy link
Author

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:

  • If I include the parameter with it's default value, I get a list of packages including the flavors.
  • If I omit the parameter, I only get the list without flavors.

@dmach
Copy link
Contributor

dmach commented Feb 25, 2022

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
Copy link
Member

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
Copy link
Member

hennevogel commented Feb 25, 2022

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
Copy link
Author

crazyscientist commented Feb 25, 2022

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:

  • ?
  • ?deleted=0

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.

Footnotes

  1. Which makes heavy use of the =x format, by the way.

@adrianschroeter
Copy link
Member

adrianschroeter commented Feb 25, 2022 via email

@crazyscientist
Copy link
Author

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
Copy link
Member

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

@adrianschroeter
Copy link
Member

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
Copy link
Author

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 added a commit to SUSE/osc-tiny that referenced this issue Aug 12, 2022
The build service has a rather specific interpretation of boolean parameters:
They are flags (i.e. without value) instead of params with explicit values.

This change explicitly tries to not send values for boolean params in order to
avoid confusing API responses.
(see e.g. openSUSE/open-build-service#9715)
boiko pushed a commit to SUSE/osc-tiny that referenced this issue Aug 12, 2022
* Handle boolean query params (fixes #72)

The build service has a rather specific interpretation of boolean parameters:
They are flags (i.e. without value) instead of params with explicit values.

This change explicitly tries to not send values for boolean params in order to
avoid confusing API responses.
(see e.g. openSUSE/open-build-service#9715)

* Fixed linter issues
crazyscientist added a commit to SUSE/osc-tiny that referenced this issue Oct 11, 2022
The `deleted` parameter interferes with other `view` parameter values, too.

This is another fallout of openSUSE/open-build-service#9715
crazyscientist added a commit to SUSE/osc-tiny that referenced this issue Oct 11, 2022
The `deleted` parameter interferes with other `view` parameter values, too.

This is another fallout of openSUSE/open-build-service#9715
@crazyscientist
Copy link
Author

crazyscientist commented Oct 12, 2022

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.

@danidoni danidoni reopened this Oct 4, 2024
danidoni added a commit to danidoni/open-build-service that referenced this issue Oct 8, 2024
This way `?deleted=0` and `?` behaves consistently, and only
`?deleted=1`
activates the behaviour for deleted projects

Fixes openSUSE#9715
danidoni added a commit to danidoni/open-build-service that referenced this issue Oct 9, 2024
This way `?deleted=0` and `?` behaves consistently, and only
`?deleted=1`
activates the behaviour for deleted projects

Fixes openSUSE#9715
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
5 participants