Project

Profile

Help

Issue #3307

import_upload: improve data validation

Added by mihai.ibanescu@gmail.com about 1 year ago. Updated 5 days ago.

Status:
CLOSED - CURRENTRELEASE
Priority:
Normal
Assignee:
Category:
-
Sprint/Milestone:
-
Severity:
2. Medium
Version:
Platform Release:
2.15.3
Blocks Release:
OS:
Backwards Incompatible:
No
Triaged:
Yes
Groomed:
No
Sprint Candidate:
No
Tags:
Pulp 2
QA Contact:
Complexity:
Smash Test:
Verified:
No
Verification Required:
No
Sprint:
Sprint 33

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

Duplicated by RPM Support - Issue #3393: API break: unit_key became mandatory during RPM upload CLOSED - DUPLICATE Actions

Associated revisions

Revision cd99f6db View on GitHub
Added by daviddavis about 1 year ago

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

Revision 2f612003 View on GitHub
Added by daviddavis about 1 year ago

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)

History

#1 Updated by mihai.ibanescu@gmail.com about 1 year 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.

#2 Updated by mihai.ibanescu@gmail.com about 1 year 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.

#3 Updated by mihai.ibanescu@gmail.com about 1 year ago

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

#4 Updated by dalley about 1 year ago

  • Project changed from RPM Support to Pulp
  • Triaged changed from No to Yes

#5 Updated by dalley about 1 year ago

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

#6 Updated by dalley about 1 year ago

  • Sprint/Milestone set to 56

Adding to sprint

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

#7 Updated by amacdona@redhat.com about 1 year 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

#8 Updated by amacdona@redhat.com about 1 year ago

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

#9 Updated by amacdona@redhat.com about 1 year ago

  • Duplicated by Issue #3393: API break: unit_key became mandatory during RPM upload added

#10 Updated by daviddavis about 1 year ago

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

#11 Updated by daviddavis about 1 year ago

  • Status changed from ASSIGNED to POST

#12 Updated by daviddavis about 1 year ago

  • Status changed from POST to MODIFIED

#13 Updated by bmbouter about 1 year ago

  • Platform Release set to 2.15.3

#14 Updated by bmbouter about 1 year ago

  • Sprint set to Sprint 33

#15 Updated by bmbouter about 1 year ago

  • Sprint/Milestone deleted (56)

#17 Updated by bmbouter about 1 year ago

  • Status changed from MODIFIED to ON_QA

#18 Updated by bmbouter about 1 year ago

  • Status changed from ON_QA to CLOSED - CURRENTRELEASE

#19 Updated by bmbouter 5 days ago

  • Tags Pulp 2 added

Please register to edit this issue

Also available in: Atom PDF