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

Asset PATCH API returns invalid assigned_to value & type in payload then GET #9427

Open
2 tasks done
0xpr03 opened this issue Apr 13, 2021 · 3 comments
Open
2 tasks done
Assignees
Labels
api backend 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick!

Comments

@0xpr03
Copy link

0xpr03 commented Apr 13, 2021

Please confirm you have done the following before posting your bug report:

Describe the bug
The patch API for assets reports the assigned_to field only as a number that could be anything.

Meanwhile the normal assets GET API returns assigned_to as an object that describes also what kind of ID we're looking at.

The further implication is that parsing an Asset is not only different between a PATCH and a GET but the returned data for a PATCH makes no sense as we get an anonymous ID.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://snipe-it.readme.io/reference#hardware-update
  2. Checkout a random Asset
  3. Do a PATCH on the asset
  4. Do a GET on the asset
  5. Inspect returned body of both and compare

Expected behavior
Asset object has the same kind of assigned_to field.

Screenshots
image
image

Server (please complete the following information):

  • Snipe-IT Version 5.1.4 and 4.9.4 - this is API v1 related so it doesn't matter and happens on either version
  • OS: Debian
  • Web Server: Apache
  • PHP Version 7.5

What you can also see is that some other fields are reported as either NULL or "" depending on whether its a PATCH payload or a GET response. Changing this would probably require a V2 of the API. Either way its a real pain to parse and requires an additional API request if you want to use the updated Asset and want to use snipeit as source of truth.

@0xpr03 0xpr03 changed the title Asset patch API returns invalid assigned_to value & type in payload Asset PATCH API returns invalid assigned_to value & type in payload than GET Apr 13, 2021
@0xpr03 0xpr03 changed the title Asset PATCH API returns invalid assigned_to value & type in payload than GET Asset PATCH API returns invalid assigned_to value & type in payload then GET Apr 13, 2021
@boring10
Copy link

There is an assigned_type field that is returned that lets you know the model (ie. User, Asset, Location). You would just need to parse it since it returns assigned_type: 'App\\Models\\User', assigned_type: 'App\\Models\\Asset', or assigned_type: 'App\\Models\\Location'.

I agree that more information being returned on the patch and delete methods would be nice though.

@0xpr03
Copy link
Author

0xpr03 commented Apr 13, 2021

Yes but these are internal declarations that shouldn't be exposed. Even worse, the API itself doesn't use what is specified in the documentation and diverges from your scheme at another section: The report part of the API is a total mess. You don't provide a App\Models\Asset for item_type you provide Asset, otherwise it won't work, so your App\\Models\\User is already a patch/delete niche and the docs for reports/activity are completely wrong. Besides you're supposed to provide the data in the body as JSON (and the demo does so) but in reality you have to provide them by URL params (and the demo thus fails).

So parts of the API are a total mess, please unify asset objects returned or leave those fields at least blank. (It doesn't make that much sense to return assigned_to when you're not supposed to set the field via PATCH, see down below.)

PATCHing the assigned_to field is a total nightmare as you can set simply a ID and the system will be in a broken state where you can't return nor checkout the asset. So it shouldn't be possible or a least sanitize it (even though we have checkin/checkout API calls that sanitize and make sure you don't mess it up). And when I'm already talking about the docs: users/me isn't documented and the bot is trying to close my issue since some time.

Last but not least: The activity report API returns localized values for the action type of history entries, giving you structures like these:

pub enum ActionType {
    #[serde(rename = "aktualisieren")]
    Update,
    #[serde(rename = "herausgeben")]
    Checkout,
    #[serde(rename = "hinzufügen")]
    Add,
    #[serde(rename = "zurücknehmen von")]
    Checkin,
    #[serde(rename = "hochgeladen")]
    Upload,
}

It's a mess to deserialize Assets if PATCH and GET return different structures about the same values, making it impossible in some languages to use a unified Object as deserialization target.

@0xpr03
Copy link
Author

0xpr03 commented May 25, 2021

Another error in the API docs: it's custom_fieldset_id not fieldset_id for PUT-ing the models. I tried the "edit docs" stuff, but I get an empty markdown field..

@snipe snipe added 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick! backend api labels Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api backend 👩‍💻 ready for dev These issues are ready for someone to work on them - take your pick!
Projects
None yet
Development

No branches or pull requests

4 participants