Prepend type for master/detail models with a plugin app label

[goosemania]

closes #4185
https://pulp.plan.io/issues/4185

Required PR: pulp/pulp_file#150

[pep8speaks]

Hello @goosemania! Thanks for updating the PR.

• There are no PEP8 issues in the file pulpcore/app/models/base.py !

• There are no PEP8 issues in the file pulpcore/tests/functional/api/using_plugin/constants.py !

Comment last updated on December 19, 2018 at 15:50 Hours UTC

[goosemania]

The failures are in pulp_file tests, the changes to pulp_file plugin are required.

[codecov]

Codecov Report

Merging #3801 into master will not change coverage.
The diff coverage is 0%.

@@          Coverage Diff           @@
##           master   #3801   +/-   ##
======================================
  Coverage    52.9%   52.9%           
======================================
  Files          65      65           
  Lines        2805    2805           
======================================
  Hits         1484    1484           
  Misses       1321    1321

Impacted Files	Coverage Δ
pulpcore/app/models/base.py	57.69% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf30f2e...adf83ff. Read the comment docs.

[daviddavis]

LGTM 👍

[dralley]

If the model is already saved, and is then modified and saved again, would this result in the app label being repeated?

[dralley]

e.g.

if self.type:
    detail_model_type = self.type
else:
    detail_model_type = self.TYPE

# detail_model_type == "pulp_file.file", because it has already been saved

self.type = '{app_label}.{type}'.format(
    app_label=self._meta.app_label,  # pulp_file
    type=detail_model_type           # pulp_file.file
)

# self.type ==  "pulp_file.pulp_file.file"

I think that line should be underneath the "else" block instead of after it.

[dralley]

@goosemania I don't quite understand why self.type needs to be re-set if it is already set. Would this be a correct way to do it?

        if not self.type:
             self.type = '{app_label}.{type}'.format(app_label=self._meta.app_label, type=self.TYPE)

[goosemania]

@dralley , I agree with your concern, thanks for pointing out. I'll update the PR.

Yeah, initially it was the way you wrote, it just means that we can't guarantee proper namespacing for types. If plugin sets a type field themselves, we are not doing anything, it can be in any format possible.
The benefit is that it's more flexible for plugin writers, though I'm not sure how critical it is for them to control the type field fully.
The downside is that we would never be able to rely on the type field to identify which plugin it came from, if we ever need to write a migration for the table related to the master model.

[daviddavis]

I think that's an acceptable risk. The plugins shouldn't set the type field themselves unless they know what they are doing and even then they are on their own if they do so.

[dralley]

See comments