hmpf / easydmp

MIT License
7 stars 2 forks source link

Export/import templates between EasyDMP instances #176

Closed hmpf closed 3 years ago

hmpf commented 3 years ago

.. or anything else capable of serving up a correctly formatted template export.

Partially fixes #55.

codecov[bot] commented 3 years ago

Codecov Report

Merging #176 (4c465fd) into master (5784c18) will increase coverage by 0.27%. The diff coverage is 66.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   55.48%   55.75%   +0.27%     
==========================================
  Files         110      112       +2     
  Lines        6016     6191     +175     
==========================================
+ Hits         3338     3452     +114     
- Misses       2678     2739      +61     
Impacted Files Coverage Δ
src/easydmp/dmpt/models.py 52.10% <32.85%> (-0.98%) :arrow_down:
src/easydmp/eestore/models.py 53.77% <60.00%> (+0.30%) :arrow_up:
src/easydmp/dmpt/admin.py 54.96% <73.68%> (+1.27%) :arrow_up:
src/easydmp/dmpt/api/v2/views.py 83.60% <83.33%> (-0.07%) :arrow_down:
src/easydmp/dmpt/export_template.py 91.93% <91.93%> (ø)
src/easydmp/dmpt/api/v2/__init__.py 100.00% <100.00%> (ø)
src/easydmp/dmpt/api/v2/serializers.py 88.46% <100.00%> (+1.50%) :arrow_up:
...dmp/dmpt/migrations/0003_TemplateImportMetadata.py 100.00% <100.00%> (ø)

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 5784c18...4c465fd. Read the comment docs.

hmpf commented 3 years ago

Happy with the export format so far. I tried using the same format as Django uses for dumpdata/loaddata, but then spectacular couldn't deal with it. We can't use dumpdata/loaddata anyway since we'll have to use new pk's everywhere.

hmpf commented 3 years ago

One needed thing is an id for an EasyDMP instance. For things publicly available, this can be a hostname. This does mean something clever is needed on localhost though. I'm considering abusing the SECRET_KEY to sign the string "localhost" then use the signature, but I guess the easiest is to have a new, explicit setting. It can then be served in the export-dump itself as well.

hmpf commented 3 years ago

As for imports, the plan is to have a new model hanging off Template containing info about the instance imported from, as well as a (json) pk-mapping from exporter to importer, so that plans can also be moved. I don't want new models with mappings per model to export, since the maps are only relevant when exporting/importing plans.

hmpf commented 3 years ago

I need to document this somewhere in case I run into it again:

The problem: Unwanted drf validators

We have an ExportSerializer that combines info about all the models we export. It outputs a big json-blob that is fed to an importer that works on python native data since every single primary key/foreign key needs changing. We will never use the ExportSerializer to save (update/create) a template and all it's dependencies. The ExportSerializer is a very nested serializer, and drf doesn't support automatic create/update on such anyway.

When writing the ExportSerializer naively, when we try to validate an export blob to import, we get tons of validation errors whenever a pk is already in use by the importing EasyDMP. These are uniqueness-errors, and happens because drf cannot know whether we want to create (trigger duplication alert!) or update (don't trigger duplication alert!). But we just need to validate to check that the blob makes sense.

The fix

I have found two ways to fix this:

1. Remove the problematic validators

There are field-specific validators that can be removed by some magic in Meta:

class SomeSerializer(serializers.ModelSerializer):
    class Meta:
        model = SomeModel
        fields = ['unique_id']
        extra_kwargs = {'unique_id': {'validators': []}}

Then there are the non-field-specific validators, like for unique_togeteher. They can be fixed like so:

class SomeSerializer(serializers.ModelSerializer):

    def get_validators(self):
        validators = getattr(getattr(self, 'Meta', None), 'validators', None)
        if validators is not None:
            return list(validators)
        return []

This allows explicit validators to still be used but remove unique_together checks and unique_for_date checks.

2. Set all fields read_only

This does not seem to affect validation!

class SomeSerializer(serializers.ModelSerializer):
    class Meta:
        model = SomeModel
        fields = ['unique_id']
        read_only_fields = fields

In the case of our ExportSerializer I opted for fix number 2.

This does make me once again wish for an obvious ReadOnlySerializer though.

hmpf commented 3 years ago

NTS: "import.py" is an illegal filename in Python. Ought to have realized it, but never needed anything like it before.

hmpf commented 3 years ago

This is ready to merge now I think. What will be done later:

hmpf commented 3 years ago

Good work. But how can coverage increase with all this untested logic?

I'll add tests for the mechanism itself next year. What I hope to do is export some of the prod templates and use them in tests.

Template-specific tests even, then we can make sure the branching/behaviour is correct. Such tests won't increase coverage, just give us peace of mind when doing radical changes, like, say: repeatable sections!

vbhavdal commented 3 years ago

The Codecov report doesn't look right to me, for example import_template.py isn't there.

hmpf commented 3 years ago

The Codecov report doesn't look right to me, for example import_template.py isn't there.

It's not added by coverage. I hope it's not some more magic due to the filename.

hmpf commented 3 years ago

cough coverage was set up to only show coverage of files that had tests.. if we turn on having it show coverage for everything cough cough we have a lot of tests to write, as coverage shrank by 4%... Let's discuss this next year ;)