Skip to content

refactor: normalize component types with ComponentType #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ormsbee merged 2 commits into from
Feb 1, 2024
Merged

refactor: normalize component types with ComponentType #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c571fb
refactor: normalize component types with ComponentType
ormsbee Jan 31, 2024
fc33d66
fix: remove incomplete comment thought
ormsbee Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion olx_importer/management/commands/load_components.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def import_block_type(self, block_type, now): # , publish_log_entry):
_component, component_version = components_api.create_component_and_version(
self.learning_package.id,
namespace=namespace,
type=block_type,
type_name=block_type,
local_key=local_key,
title=display_name,
created=now,
Expand Down
7 changes: 3 additions & 4 deletions openedx_learning/core/components/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,15 @@ class ComponentAdmin(ReadOnlyModelAdmin):
"""
Django admin configuration for Component
"""
list_display = ("key", "uuid", "namespace", "type", "created")
list_display = ("key", "uuid", "component_type", "created")
readonly_fields = [
"learning_package",
"uuid",
"namespace",
"type",
"component_type",
"key",
"created",
]
list_filter = ("type", "learning_package")
list_filter = ("component_type", "learning_package")
search_fields = ["publishable_entity__uuid", "publishable_entity__key"]
inlines = [ComponentVersionInline]

Expand Down
48 changes: 30 additions & 18 deletions openedx_learning/core/components/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,32 +18,44 @@
from django.db.models import Q, QuerySet
from django.db.transaction import atomic

from ...lib.cache import lru_cache
from ..publishing import api as publishing_api
from .models import Component, ComponentVersion, ComponentVersionRawContent
from .models import Component, ComponentType, ComponentVersion, ComponentVersionRawContent


@lru_cache(maxsize=128)
def get_or_create_component_type_id(namespace: str, name: str) -> int:
Copy link
Member

@kdmccormick kdmccormick Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it returns a pk, I think I would mildly prefer get_or_create_component_type, since you're going to create a "compoment type", not a "component type id".

Copy link
Contributor Author

@ormsbee ormsbee Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not great, but I decided to keep it with _id at the end because of all the places we're returning a full model.

Honestly, I'm currently mulling over how this part works in my current PR. I realized that the way things are now, the cache is vulnerable to getting corrupted when there are errors, since we might cache a value for a ComponentType that was later rolled back. So I might have to rethink this setup entirely. But I'll do that in the next PR. 😛

Copy link
Member

@kdmccormick kdmccormick Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Would the rolled-back id be mis-cached in the lru_cache, or somewhere else?

Copy link
Contributor Author

@ormsbee ormsbee Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in lru_cache.

Copy link
Contributor

@bradenmacdonald bradenmacdonald Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't expect that creating ComponentType instances will occur outside of create_component. So I suggest that you just put the ComponentType.objects.get_or_create( call inline (without a cache) inside create_component. Then if it gets rolled back, no problem.

If this gets simplified to @lru_cache def get_component_type_id() then you have two nice simplifications:

  1. If the ComponentType is not found, it won't be cached, due to the ComponentType.NotFound exception

  2. You can optimize lookups like this:

     Component.with_publishing_relations \
                    .get(
                        learning_package_id=learning_package_id,
                        component_type__namespace=namespace,
                        component_type__name=type_name,
                        local_key=local_key,
                    )

    becomes

     Component.with_publishing_relations \
                    .get(
                        learning_package_id=learning_package_id,
                        component_type_id=get_component_type_id(namespace, type_name),
                        local_key=local_key,
                    )

which will then cause your function to throw differentiated exceptions: Component.NotFound OR ComponentType.NotFound vs. only throwing Component.NotFound. Arguably more explicit (but maybe a pain if callers need to plan to catch both).

Copy link
Member

@kdmccormick kdmccormick Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Nice, I like that.

Copy link
Contributor Author

@ormsbee ormsbee Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that I'm going to just kick the lru to the curb entirely for now. The worry I have is that the lru bleeds across transactions and probably even threads (?), and we don't know what combination of reads and writes of Components people are going to do in a big transaction (e.g. import). So it's possible that a process will add a bunch of stuff (including new ContentTypes), then do some reads of Components because they're grouping them somehow (thus loading things into the LRU cache). Then their process dies, and gets rolled back.

I guess the semantics I'm really looking for are transaction-local caches. Which actually sounds really cool now that I think about it... but I'm definitely not going there this week. A much simpler version of that would be a local object that proxies the requests through and goes out of scope at the end of whatever import operation happens. But things will probably be fine if I just punt on caching for a while.

"""
Get the ID of a ComponentType, and create if missing.
"""
component_type, _created = ComponentType.objects.get_or_create(
namespace=namespace,
name=name,
)
return component_type.id


def create_component(
learning_package_id: int,
/,
namespace: str,
type: str, # pylint: disable=redefined-builtin
type_name: str,
local_key: str,
created: datetime,
created_by: int | None,
) -> Component:
"""
Create a new Component (an entity like a Problem or Video)
"""
key = f"{namespace}:{type}@{local_key}"
key = f"{namespace}:{type_name}@{local_key}"
with atomic():
publishable_entity = publishing_api.create_publishable_entity(
learning_package_id, key, created, created_by
)
component = Component.objects.create(
publishable_entity=publishable_entity,
learning_package_id=learning_package_id,
namespace=namespace,
type=type,
component_type_id=get_or_create_component_type_id(namespace, type_name),
local_key=local_key,
)
return component
Expand Down Expand Up @@ -163,7 +175,7 @@ def create_component_and_version(
learning_package_id: int,
/,
namespace: str,
type: str, # pylint: disable=redefined-builtin
type_name: str,
local_key: str,
title: str,
created: datetime,
Expand All @@ -174,7 +186,7 @@ def create_component_and_version(
"""
with atomic():
component = create_component(
learning_package_id, namespace, type, local_key, created, created_by
learning_package_id, namespace, type_name, local_key, created, created_by
)
component_version = create_component_version(
component.pk,
Expand All @@ -199,7 +211,7 @@ def get_component_by_key(
learning_package_id: int,
/,
namespace: str,
type: str, # pylint: disable=redefined-builtin
type_name: str,
local_key: str,
) -> Component:
"""
Expand All @@ -208,8 +220,8 @@ def get_component_by_key(
return Component.with_publishing_relations \
.get(
learning_package_id=learning_package_id,
namespace=namespace,
type=type,
component_type__namespace=namespace,
component_type__name=type_name,
local_key=local_key,
)

Expand All @@ -218,7 +230,7 @@ def component_exists_by_key(
learning_package_id: int,
/,
namespace: str,
type: str, # pylint: disable=redefined-builtin
type_name: str,
local_key: str
) -> bool:
"""
Expand All @@ -228,10 +240,10 @@ def component_exists_by_key(
no current Draft version for it), or if it's been unpublished.
"""
try:
_component = Component.objects.only('pk').get(
_component = Component.objects.only('pk', 'component_type').get(
learning_package_id=learning_package_id,
namespace=namespace,
type=type,
component_type__namespace=namespace,
component_type__name=type_name,
local_key=local_key,
)
return True
Expand All @@ -245,7 +257,7 @@ def get_components(
draft: bool | None = None,
published: bool | None = None,
namespace: str | None = None,
types: list[str] | None = None,
type_names: list[str] | None = None,
draft_title: str | None = None,
published_title: str | None = None,
) -> QuerySet[Component]:
Expand All @@ -265,9 +277,9 @@ def get_components(
if published is not None:
qset = qset.filter(publishable_entity__published__version__isnull=not published)
if namespace is not None:
qset = qset.filter(namespace=namespace)
if types is not None:
qset = qset.filter(type__in=types)
qset = qset.filter(component_type__namespace=namespace)
if type_names is not None:
qset = qset.filter(component_type__name__in=type_names)
if draft_title is not None:
qset = qset.filter(
publishable_entity__draft__version__title__icontains=draft_title
Expand Down
29 changes: 22 additions & 7 deletions openedx_learning/core/components/migrations/0001_initial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Generated by Django 3.2.23 on 2024-01-22 00:38
# Generated by Django 3.2.23 on 2024-01-31 05:34

import uuid

Expand All @@ -13,25 +13,30 @@ class Migration(migrations.Migration):
initial = True

dependencies = [
('oel_contents', '0001_initial'),
('oel_publishing', '0001_initial'),
('oel_contents', '0001_initial'),
]

operations = [
migrations.CreateModel(
name='Component',
fields=[
('publishable_entity', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, primary_key=True, serialize=False, to='oel_publishing.publishableentity')),
('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)),
('type', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)),
('local_key', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=500)),
('learning_package', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage')),
],
options={
'verbose_name': 'Component',
'verbose_name_plural': 'Components',
},
),
migrations.CreateModel(
name='ComponentType',
fields=[
('id', models.AutoField(primary_key=True, serialize=False)),
('namespace', openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)),
('name', openedx_learning.lib.fields.MultiCollationCharField(blank=True, db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, max_length=100)),
],
),
migrations.CreateModel(
name='ComponentVersion',
fields=[
Expand Down Expand Up @@ -59,6 +64,16 @@ class Migration(migrations.Migration):
name='raw_contents',
field=models.ManyToManyField(related_name='component_versions', through='oel_components.ComponentVersionRawContent', to='oel_contents.RawContent'),
),
migrations.AddField(
model_name='component',
name='component_type',
field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='oel_components.componenttype'),
),
migrations.AddField(
model_name='component',
name='learning_package',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='oel_publishing.learningpackage'),
),
migrations.AddIndex(
model_name='componentversionrawcontent',
index=models.Index(fields=['raw_content', 'component_version'], name='oel_cvrawcontent_c_cv'),
Expand All @@ -73,10 +88,10 @@ class Migration(migrations.Migration):
),
migrations.AddIndex(
model_name='component',
index=models.Index(fields=['learning_package', 'namespace', 'type', 'local_key'], name='oel_component_idx_lc_ns_t_lk'),
index=models.Index(fields=['component_type', 'local_key'], name='oel_component_idx_ct_lk'),
),
migrations.AddConstraint(
model_name='component',
constraint=models.UniqueConstraint(fields=('learning_package', 'namespace', 'type', 'local_key'), name='oel_component_uniq_lc_ns_t_lk'),
constraint=models.UniqueConstraint(fields=('learning_package', 'component_type', 'local_key'), name='oel_component_uniq_lc_ct_lk'),
),
]
92 changes: 62 additions & 30 deletions openedx_learning/core/components/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,45 @@
from ..publishing.models import LearningPackage


class ComponentType(models.Model):
"""
Normalized representation of a type of Component.

The only namespace being used initially will be 'xblock.v1', but we will
probably add a few others over time, such as a component type to represent
packages of files for things like Files and Uploads or python_lib.zip files.

Make a ForeignKey against this table if you have to set policy based on the
type of Components–e.g. marking certain types of XBlocks as approved vs.
experimental for use in libraries.
"""
id = models.AutoField(primary_key=True)

# namespace and name work together to help figure out what Component needs
# to handle this data. A namespace is *required*. The namespace for XBlocks
# is "xblock.v1" (to match the setup.py entrypoint naming scheme).
namespace = case_sensitive_char_field(max_length=100, blank=False)

# name is a way to help sub-divide namespace if that's convenient. This
# field cannot be null, but it can be blank if it's not necessary. For an
# XBlock, this corresponds to tag, e.g. "video". It's also the block_type in
# the UsageKey.
name = case_sensitive_char_field(max_length=100, blank=True)

constraints = [
models.UniqueConstraint(
fields=[
"namespace",
"name",
],
name="oel_component_type_uniq_ns_n",
),
]

def __str__(self):
return f"{self.namespace}:{self.name}"


class Component(PublishableEntityMixin): # type: ignore[django-manager-missing]
"""
This represents any Component that has ever existed in a LearningPackage.
Expand Down Expand Up @@ -63,7 +102,7 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing]
-----------------

The ``key`` field on Component's ``publishable_entity`` is dervied from the
``(namespace, type, local_key)`` fields in this model. We don't support
``component_type`` and ``local_key`` fields in this model. We don't support
changing the keys yet, but if we do, those values need to be kept in sync.

How build on this model
Expand All @@ -75,9 +114,12 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing]
# Tell mypy what type our objects manager has.
# It's actually PublishableEntityMixinManager, but that has the exact same
# interface as the base manager class.
objects: models.Manager[Component]
objects: models.Manager[Component] = WithRelationsManager(
'component_type'
)

with_publishing_relations = WithRelationsManager(
with_publishing_relations: models.Manager[Component] = WithRelationsManager(
'component_type',
'publishable_entity',
'publishable_entity__draft__version',
'publishable_entity__draft__version__componentversion',
Expand All @@ -93,53 +135,43 @@ class Component(PublishableEntityMixin): # type: ignore[django-manager-missing]
# columns from different tables).
learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE)

# namespace and type work together to help figure out what Component needs
# to handle this data. A namespace is *required*. The namespace for XBlocks
# is "xblock.v1" (to match the setup.py entrypoint naming scheme).
namespace = case_sensitive_char_field(max_length=100, blank=False)
# What kind of Component are we? This will usually represent a specific
# XBlock block_type, but we want it to be more flexible in the long term.
component_type = models.ForeignKey(ComponentType, on_delete=models.PROTECT)

# type is a way to help sub-divide namespace if that's convenient. This
# field cannot be null, but it can be blank if it's not necessary. For an
# XBlock, type corresponds to tag, e.g. "video". It's also the block_type in
# the UsageKey.
type = case_sensitive_char_field(max_length=100, blank=True)

# local_key is an identifier that is local to the (namespace, type). The
# publishable.key should be calculated as a combination of (namespace, type,
# local_key).
# local_key is an identifier that is local to the learning_package and
# component_type. The publishable.key should be calculated as a
# combination of component_type and local_key.
local_key = key_field()

class Meta:
constraints = [
# The combination of (namespace, type, local_key) is unique within
# The combination of (component_type, local_key) is unique within
# a given LearningPackage. Note that this means it is possible to
# have two Components that have the exact same local_key. An XBlock
# would be modeled as namespace="xblock.v1" with the type as the
# block_type, so the local_key would only be the block_id (the
# very last part of the UsageKey).
# have two Components in the same LearningPackage to have the same
# local_key if the component_types are different. So for example,
# you could have a ProblemBlock and VideoBlock that both have the
# local_key "week_1".
models.UniqueConstraint(
fields=[
"learning_package",
"namespace",
"type",
"component_type",
"local_key",
],
name="oel_component_uniq_lc_ns_t_lk",
name="oel_component_uniq_lc_ct_lk",
),
]
indexes = [
# Global Namespace/Type/Local-Key Index:
# Global Component-Type/Local-Key Index:
# * Search by the different Components fields across all Learning
# Packages on the site. This would be a support-oriented tool
# from Django Admin.
models.Index(
fields=[
"learning_package",
"namespace",
"type",
"component_type",
"local_key",
],
name="oel_component_idx_lc_ns_t_lk",
name="oel_component_idx_ct_lk",
),
]

Expand All @@ -148,7 +180,7 @@ class Meta:
verbose_name_plural = "Components"

def __str__(self):
return f"{self.namespace}:{self.type}:{self.local_key}"
return f"{self.component_type.namespace}:{self.component_type.name}:{self.local_key}"


class ComponentVersion(PublishableEntityVersionMixin):
Expand Down
Loading