Copy/pasting a component that has no explicitly set display_name will show the unit's display name

[bradenmacdonald]

Bug:

1. Create a new Drag and Drop v2 component. Do not make any edits to it. Note its display name is "Drag and Drop"
   • We heard this is also happening with open response blocks, and probably other XBlock types too.
2. Copy it and paste it anywhere.
3. The newly pasted Drag and Drop will have the same display_name as the parent unit it gets pasted into.

Reason:

The bug is occurring here:

https://github.com/openedx/edx-platform/blob/b9be2b1e563a974fabc818d489d79367c68df98b/cms/djangoapps/contentstore/views/helpers.py#L328-L342

The problem is with pasting the OLX, not with the copying. The copied OLX is correct (basically just <drag-and-drop-v2 />).

As has bit me before, the XBlock API as used in edx-platform has a chicken-and-egg problem: you need to have an initialized runtime in order to call xblock_class.parse_xml(node, runtime, ...) to create an XBlock, but you need to have an existing XBlock in order to correctly initialize the LMS or Studio runtimes, because they have some internal state specific to the block that's being loaded.

What we're doing to implement "paste from OLX" is loading the vertical block that will become the parent of the newly pasted drag-and-drop-v2 block, then using that parent vertical's runtime to create the child. This should be fine per the XBlock API. However, the problem is that when parse_xml calls runtime.construct_xblock_from_class(xblock_class, keys) to create a new child, the child runtime's field-data service remains bound to the parent block:

xblock_class = runtime.load_block_type("drag-and-drop-v2")
temp_xblock = runtime.construct_xblock_from_class(xblock_class, keys)
temp_xblock.runtime.service(temp_xblock, 'field-data')._authored_data._kvs._definition.definition_locator
# def-v1:6446d555e6d042b1db5d9c81+type@vertical

I believe this is because the core/vanilla XBlock API assumes that a runtime can be shared among blocks, and calling temp_xblock.runtime.service(temp_xblock, 'field-data') is always supposed to give a version of the service bound to temp_xblock, not one bound to whatever block the runtime was instantiated with. In fact, even the field data API generally assumes that the KVS is not "bound" to any particular block but used by any active blocks in memory.

So arguably the current PreviewModuleSystem/Runtime has bugs in both construct_xblock_from_class (which should return a fresh XBlock instance with its own separate runtime that's correctly bound to that new XBlock) and .service() (which should never return the 'field-data' service of the parent block when the child block is explicitly passed in as the first arg).

Suggested approaches to fixing it:

1. Override construct_xblock_from_class in the runtime, so that it will create a new runtime instance for the newly constructed block.
2. Fix runtime.service() in edx-platform so that runtime.service(temp_xblock, 'field-data') cannot return the service of another block like it's doing now.
3. Modify SplitMongoKVS so that it doesn't take a specific definition locator in its constructor but can work with any definition (risky)
4. Do a hacky patch to replace or re-bind the field-data service after we call construct_xblock_from_class - this is probably not possible unfortunately because that happens within parse_xml and XBlocks need to be able to override parse_xml to do custom XML parsing.
5. Possibly use an entirely different runtime for parsing the XML and getting the field data, then use a pure modulestore API to create the new item and set its field data.

Also note that for some reason this doesn't seem to affect the XBlocks included with the platform (the ones that have XModule heritage); they seem to be using a different way of getting field data that doesn't involve the field-data service.

[bradenmacdonald]

@ormsbee Any thoughts on which way to fix this? Or @Agrendalath?

[Agrendalath]

@bradenmacdonald, thank you for the detailed description of this bug! We had a similar issue (in that case, with the runtime initialization order) in openedx/openedx-platform#32165. I like the idea from @ormsbee to remove block-specific code from the runtime initialization:

I think that as a follow-up to this, I'd like to discuss the feasibility of removing block-specific handling from prepare_runtime_for_user, so that it becomes something that only needs to be initialized once for a student during a request, instead of run for each XBlock being invoked.

If we make XBlocks initialize their own field-data, it should resolve the issue. I will add this as a proposed approach in the BD-13's follow-up, but it won't be a quick fix. I don't know how urgent is resolving the current issue.

Override construct_xblock_from_class in the runtime, so that it will create a new runtime instance for the newly constructed block.

What would be the performance impact of this change? Or would we use it only when this method is invoked from parse_xml?

Fix runtime.service() in edx-platform so that runtime.service(temp_xblock, 'field-data') cannot return the service of another block like it's doing now.

This could be nice. We should be able to create a separate space for services of each XBlock that reinitialized the runtime (if there is a way to preserve services between these reinitializations).

I'm unsure about the implications and side effects of other approaches.

[ormsbee]

If we make XBlocks initialize their own field-data, it should resolve the issue. I will add this as a proposed approach in the BD-13's follow-up, but it won't be a quick fix. I don't know how urgent is resolving the current issue.

This is very urgent to me, and I'm still seriously considering reverting the big BD-13 PRs if we can't make progress towards getting this resolved soon. Not because of this specific bug, but because I'm worried there are more dangerous ones that we just aren't seeing yet.

Override construct_xblock_from_class in the runtime, so that it will create a new runtime instance for the newly constructed block.

I think this is the way to go for now–wasteful, but safer.

Fix runtime.service() in edx-platform so that runtime.service(temp_xblock, 'field-data') cannot return the service of another block like it's doing now.

Wouldn't it just blow up in that case, for this situation?