Project

Profile

Help

Issue #3307

closed

import_upload: improve data validation

Added by mihai.ibanescu@gmail.com almost 7 years ago. Updated over 5 years ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Sprint/Milestone:
-
Start date:
Due date:
Estimated time:
Severity:
2. Medium
Version:
Platform Release:
2.15.3
OS:
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
Sprint:
Sprint 33
Quarter:

Description

As a user, uploading a unit using the REST API allows me to specify optional unit_key and unit_metadata.

https://docs.pulpproject.org/dev-guide/integration/rest-api/content/upload.html#import-into-a-repository

In pulp 2.10, unit_key was permitted to be None (looking at /usr/lib/python2.7/site-packages/pulp_rpm/plugins/importers/yum/upload.py _handle_package)

    model_class = plugin_api.get_unit_model_by_id(type_id)
    update_fields_inbound(model_class, unit_key or {})
    update_fields_inbound(model_class, metadata or {})
    ...
    # Update the RPM-extracted data with anything additional the user specified.
    # Allow the user-specified values to override the extracted ones.
    rpm_data.update(metadata or {})
    rpm_data.update(unit_key or {})

    # Validate the user specified data by instantiating the model
    try:
        unit = model_class(**rpm_data)
    except TypeError:
        raise ModelInstantiationError()

It looks like in pulp 2.15 it is required that it is a dict.

    # Update the RPM-extracted data with anything additional the user specified.
    # Allow the user-specified values to override the extracted ones.
    for key, value in metadata.items():
        setattr(unit, key, value)
    for key, value in unit_key.items():
        setattr(unit, key, value)

Change was introduced in https://github.com/pulp/pulp_rpm/commit/62db898a81679b40e3cc3a48af9f3b830f4521cb#diff-12687b797580f9b515df98568ed0efe6R418

Notice how it is now expected that unit_key has an items method, whereas the previous code was working even with None.


Related issues

Has duplicate RPM Support - Issue #3393: API break: unit_key became mandatory during RPM uploadCLOSED - DUPLICATEActions
Actions #1

Updated by mihai.ibanescu@gmail.com almost 7 years ago

I am not sure what my expectations are for this.

The REST API documentation should make it clear that unit_key and unit_metadata should be Dict, not the overly generic object.

It would be nice if the input data was validated, because once again the error I was seeing was the very unhelpful: "/usr/lib/python2.7/site-packages/pulp/server/managers/content/upload.py\", line 223, in import_uploaded_unit\n unit_type=unit_type_id, summary=result['summary'], details=result['details']\nPulpCodedException: The importer yum_importer indicated a failed response when uploading rpm unit to repository "

Only slightly more information was present in the task (if you dumped the json with pulp-admin -v -v):

"error": {
    "code": "PLP0047", 
    "data": {
      "unit_type": "rpm", 
      "importer_id": "yum_importer", 
      "repo_id": "_session-x64_redhat_linux_6-yum-20180120012852.593262-yum", 
      "details": {
        "errors": [
          "unexpected error occurred importing uploaded file: 'NoneType' object has no attribute 'items'"
        ]
      }, 
      "summary": ""
    }, 

Because I currently have client libraries out in the wild that rely on this behavior, I would need it fixed server-side. But I can patch it myself until all my clients catch up.

Making Pulp easier to debug when these problems occur would be the biggest take-away. Having to put breakpoints in a live system is just not right. In this particular case, I believe something was indeed being logged to the journal, but in other cases it's total silence.

Actions #2

Updated by mihai.ibanescu@gmail.com almost 7 years ago

Pulp Core could validate that unit_key and unit_metadata are indeed dict objects. Somewhere around here:

https://github.com/pulp/pulp/blob/master/server/pulp/server/managers/content/upload.py#L196

That way, the plugins are guaranteed a uniform interface.

Actions #3

Updated by mihai.ibanescu@gmail.com almost 7 years ago

  • Subject changed from import_upload: unit_key can no longer be None to import_upload: improve data validation
Actions #4

Updated by dalley almost 7 years ago

  • Project changed from RPM Support to Pulp
  • Triaged changed from No to Yes
Actions #5

Updated by dalley almost 7 years ago

  • Related to Issue #3393: API break: unit_key became mandatory during RPM upload added
Actions #6

Updated by dalley almost 7 years ago

  • Sprint/Milestone set to 56

Adding to sprint

Duplicate issue here https://pulp.plan.io/issues/3393

Actions #7

Updated by amacdona@redhat.com almost 7 years ago

To fix backwards compatibility [0], unit_key should allow a null value, which should default to {} internally.

[0]: https://pulp.plan.io/issues/3393

Actions #8

Updated by amacdona@redhat.com almost 7 years ago

  • Related to deleted (Issue #3393: API break: unit_key became mandatory during RPM upload)
Actions #9

Updated by amacdona@redhat.com almost 7 years ago

  • Has duplicate Issue #3393: API break: unit_key became mandatory during RPM upload added
Actions #10

Updated by daviddavis almost 7 years ago

  • Project changed from Pulp to RPM Support
  • Status changed from NEW to ASSIGNED
  • Assignee set to daviddavis

Added by daviddavis almost 7 years ago

Revision cd99f6db | View on GitHub

Making unit_key a non-required field

None was a valid value for unit_key until a commit began requiring it. This fixes the backward incompatible change.

fixes #3307 https://pulp.plan.io/issues/3307

Actions #11

Updated by daviddavis almost 7 years ago

  • Status changed from ASSIGNED to POST
Actions #12

Updated by daviddavis almost 7 years ago

  • Status changed from POST to MODIFIED
Actions #13

Updated by bmbouter almost 7 years ago

  • Platform Release set to 2.15.3
Actions #14

Updated by bmbouter almost 7 years ago

  • Sprint set to Sprint 33
Actions #15

Updated by bmbouter almost 7 years ago

  • Sprint/Milestone deleted (56)

Added by daviddavis almost 7 years ago

Revision 2f612003 | View on GitHub

Making unit_key a non-required field

None was a valid value for unit_key until a commit began requiring it. This fixes the backward incompatible change.

fixes #3307 https://pulp.plan.io/issues/3307

(cherry picked from commit cd99f6db0aced4537aa4ff6ed95aa4360091ab4e)

Actions #17

Updated by bmbouter almost 7 years ago

  • Status changed from MODIFIED to 5
Actions #18

Updated by bmbouter almost 7 years ago

  • Status changed from 5 to CLOSED - CURRENTRELEASE
Actions #19

Updated by bmbouter over 5 years ago

  • Tags Pulp 2 added

Also available in: Atom PDF