Bugfix - fix handling of type_ids in remove_content

[AvlWx2014]

Overview

Fix a bug in Repository#remove_content causing all content to be removed from a repository, even when one or more content type IDs are passed to it.

From the Pulp 2.x docs, the body of an unassociate request should be a criteria object of which type_ids is a field.

The current implementation adds

{"type_ids": [...]}

to the request body, when it should be

{"criteria": {"type_ids": [...]}}

This is also consistent with other parts of the codebase e.g. the Search API, which correctly adds the type_ids field to a parent criteria object.

Approach

Rather than working with raw dicts and hard-coded string keys, my initial approach was to model the criteria object with a new data class called CriteriaDocument that encapsulates necessary fields, and knows how to serialize itself to JSON for the request body.

Eventually I noticed that the search API uses existing Criteria and Matcher classes, as well as some helper functions to do the same thing. In fact, there is an open issue that asks to extend Repository#remove_content to accept and handle Criteria. After seeing this, it made sense to try and use existing APIs to fix this bug and start moving in that direction.

Unfortunately, there seems to be some complex handling of Criteria in the search API in order to properly serialize them for client requests due to the need to incorporate MongoDB query syntax. It is unclear how much of that extra handling of Criteria is necessary to properly extend remove_content to accept Criteria. If most or all of the same handling is needed, then it would be beneficial to extract the business logic to a couple classes, or to possibly rename some existing helper functions (current helpers are named specifically for the search API) to avoid duplicating code and/or sewing confusion (e.g. remove_content using a function called search_for_criteria is a bit confusing).

The updated implementation here prefers a Criteria object passed in using the criteria kwarg, but falls back to the old type_ids kwarg if no Criteria is present. Some changes to both Client and FakeClient were made to work with a Criteria instance rather than a list of content type IDs. test_remove_content.py needed to be updated to reflect the correct expected value of the unassociate request body.

[rohanpm]

I only see one issue with the code changes, but there are some issues with the PR:

• CI fails - ignore this for now, I think it might be bogus, I'll investigate it a bit.
• the chain of commits has some ideas you tried and then abandoned. Can you please revise the history to drop some of those commits? e.g. "git rebase -i"
• formatting changes are being mixed with code changes. Can you please avoid that? Note, "git add -p" is helpful to pick which parts of a file to commit.

With respect to "organize imports" in particular, I'm not confident that what pycharm does will be the same as what other tools and IDEs are doing. If you want to apply some automated sorting of the imports, can you please instead do it by 'isort' and integrating that with pre-commit tool - here's an example, https://github.com/release-engineering/pubtools/blob/main/.pre-commit-config.yaml . It means all the conventions for source code formatting in the repo are under the control of pre-commit, and everyone can follow them regardless of IDE as the exact tools and versions are encoded in the config.

If you want to pursue that I'd suggest opening a separate PR which could be submitted first.

[rohanpm]

Internally making use of a 'criteria' object seems like a good idea, but I'd rather we don't accept a criteria argument to this function yet. Can we please drop this so the "if criteria is None" code path is the only path?

This is public API, if we support 'criteria' we need to at least document it, also document and test which types do/don't work. I think it's premature to do that if we're not ready to support it "properly" yet.

[AvlWx2014]

Good point, I have made the change to drop a criteria kwarg and instead stick with type_ids.

[rohanpm]

• CI fails - ignore this for now, I think it might be bogus, I'll investigate it a bit.

Should be fixed by #106 .

[AvlWx2014]

With respect to "organize imports" in particular, I'm not confident that what pycharm does will be the same as what other tools and IDEs are doing. If you want to apply some automated sorting of the imports, can you please instead do it by 'isort' and integrating that with pre-commit tool

I believe PyCharm by default organizes imports alphabetically, and groups them based on PEP-8 guidelines. I would hope other IDEs also do this too, but I agree it makes more sense to use a standard tool (which looks like it does the same thing) so I will take a look at adding isort to the pre-commit config. Happily, it seems isort has tried very hard to be compatible with Black, which is a plus.

[rohanpm]

Since #106 was submitted, could you please rebase this to get a clean Travis CI run?

[AvlWx2014]

@rohanpm et al - I do not appear to have permissions to perform the merge so feel free to pull the trigger when you think is appropriate