apptension / saas-boilerplate

SaaS Boilerplate - Open Source and free SaaS stack that lets you build SaaS products faster in React, Django and AWS. Focus on essential business logic instead of coding repeatable features!
https://apptension.com/saas-boilerplate
MIT License
2.18k stars 253 forks source link

Seemingly pointless insertion of DB records in Contentful sync worker #564

Open hirurana opened 4 months ago

hirurana commented 4 months ago

Describe the bug

Was looking through the contentful async worker code (packages/workers/content/services.py) and noticed that the sync of contentful entries seems to add new entries twice (if I understand this correctly).

The worker will call the sync() method within the ContentfulSync class which calls self.sync_entries().

def sync(self):
        self.sync_entries()
        self.unpublish_missing_entries()

This is defined as:

def sync_entries(self):
        skip = 0
        limit = 1000

        instances = []
        while True:
            entries = self.client.entries({'skip': skip, 'limit': limit, 'include': 0})
            skip += 1

            if not entries:
                break

            page_instances = [self.sync_entry(entry) for entry in entries]
            instances += list(filter(lambda x: x is not None, page_instances))

        self.session.add_all(instances)

we can see that this is essentially a wrapper to add multiple new entries into the different content models as it calls self.sync_entry(entry)

But this class method is defined as:

def sync_entry(self, entry: contentful.Entry):
        Model = self.get_db_model(entry.content_type)
        if Model is None:
            return None

        fields = {}
        for field_name, field in entry.fields().items():
            if isinstance(field, contentful.Link):
                fields[field_name] = field.sys
            else:
                fields[field_name] = field

        instance = Model(id=entry.sys['id'], fields=fields, is_published=True)
        instance = self.session.merge(instance)
        self.session.commit()
        self.entry_ids.add(entry.id)
        return instance

So question here is isn't the self.session.add_all(instances) in sync_entries() redundant as these records are already being added in sync_entry()?

Steps to reproduce

Go to file packages/workers/content/services.py within ContentfulSync

System Info

n/a

Logs

No response

Validations