feat!: remove block handling from runtime initialization of ReplaceURLService [FC-0026]

[Agrendalath]

Description

The goal of FC-0026 is to remove the XBlock-specific handling from the prepare_runtime_for_user function. This part handles the ReplaceURLService.

Supporting information

Private-ref: BB-7448

Testing instructions

1. Install xblock-utils from fix!: use TinyMCE 5 as HTML editor openedx-unsupported/xblock-utils#207.
2. Install edx-sga==0.22.0 (refactor!: remove deprecated runtime attributes [FC-0026] mitodl/edx-sga#339).
3. Set up the edx-sga XBlock and test that replacing URLs in the Solution field:
   1. If you're using the Demo course, add <img src="/static/images_course_image.jpg" /> and check that the image is rendered in Studio, Preview, and LMS. If you're using a custom course, you can upload an image under Content -> Files & Uploads in Studio.
   2. Within the demo course, add the <a href="/jump_to_id/vertical_0270f6de40fc">link</a> link and see if it's redirecting users to the "Introduction: Video and Sequences" unit in LMS and Preview. This feature doesn't work in Studio.

Other information

This change is backward-incompatible with the deprecated shims for replacing URLs directly from the runtime. We're removing them to make the runtime block-agnostic.

[openedx-webhooks]

Thanks for the pull request, @Agrendalath!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

[Agrendalath]

This is an interesting part. Removing the call to block.static_asset_path changed where the field-data cache is initialized. This altered the block runtime initialization order, and the staff_debug_info became overridden by parent blocks. However, adding this missing argument here fixes the issue.

Commit with a temporary fix I used for the verification: open-craft@655e104.

[Agrendalath]

Actually, we should not pass this, as it hides staff debug buttons in other places (which is not caught by tests). See the description of the HACK in the PR.

[Agrendalath]

@0x29a, this is ready for review.

[0x29a]

👍

• I tested this: set up edx-sga and checked that both image and link are replaced and rendered / work correctly.
• I read through the code.
• Includes documentation

[Agrendalath]

@bradenmacdonald, @ormsbee, this is ready for your check.

[bradenmacdonald]

The code looks good to me, though I'm a little concerned about this hack for the field_data/parent blocks. I don't quite understand the issue. But your approach seems fine if it will be temporary.

By the way, it seems like we're now initializing a ton of services for every XBlock, regardless of whether or not they actually get used. I know the services are pretty lightweight but it might be good in the future to check block.service_declaration(...) before initializing services, or initialize them on demand like I did in the new runtime.

[bradenmacdonald]

So we don't want the parent XBlock's runtimes to be bound to user-specific data?

Also can you clarify exactly what is meant by "until we remove all XBlock-specific code from here" ?

[Agrendalath]

@bradenmacdonald, I'll answer all your comments here to keep the context in a single place, as these items are related.

Part 1: Explanation of the problem

The code looks good to me, though I'm a little concerned about this hack for the field_data/parent blocks. I don't quite understand the issue. But your approach seems fine if it will be temporary.

I added an explanation to this part as a HACK because the block.static_asset_path expression does not make sense when we don't pass its value anywhere. The gist of this approach is - do not change the existing behavior of accessing field_data with this PR.

Without this PR, we are accessing the static_asset_path here: https://github.com/openedx/edx-platform/blob/9fbc263edd5215b58d02b6c085425d8c41128e96/lms/djangoapps/courseware/block_render.py#L518-L523

With the changes in this PR, we're moving this access here:
https://github.com/openedx/edx-platform/blob/da017f9798c6d74925584ed1a96943958966808f/common/djangoapps/static_replace/services.py#L57-L63

This seems like a harmless change, as we only access an XBlock field. However, this field comes from the InheritanceMixin:
https://github.com/openedx/edx-platform/blob/f13263df4f0e9aff48fbbb7ca04583619317b947/xmodule/modulestore/inheritance.py#L133-L138

Therefore, to access it, we're traversing the course structure to check if the XBlock's parents declare a value for this field. Accessing an XBlock's parent looks like this:
https://github.com/openedx/XBlock/blob/7b8bdeafbabbbc96f11dfbceaf53c89bd416ea6f/xblock/mixins.py#L369-L377

As we can see, parents are cached once they are obtained for the first time. Let's return to the failing test I mentioned in this HACK description. This test uses the render_xblock view. A part of this view is running the enclosing_sequence_for_gating_checks. This function looks for an XBlock's parent, which is a SequenceBlock/sequential/subsection (these terms are used interchangeably):
https://github.com/openedx/edx-platform/blob/1b50e804375519a5e8cc3239a198ae485c6521df/lms/djangoapps/courseware/views/views.py#L1484-L1486

As you can see, this loop uses the get_parent method I mentioned above. However, let's take a look at the lines below:
https://github.com/openedx/edx-platform/blob/1b50e804375519a5e8cc3239a198ae485c6521df/lms/djangoapps/courseware/views/views.py#L1488-L1491

The thing is - the comment you see in this part of the code is only partially true. When we unified the ModuleSystem and the DescriptorSystem, we needed to find a way to know when to bind the user's data to the XBlock. We used the following approach - when the runtime we're using to initialize another XBlock already has bound the user's data - we're binding it for the new XBlock, too:
https://github.com/openedx/edx-platform/blob/da017f9798c6d74925584ed1a96943958966808f/xmodule/x_module.py#L1473-L1477

As we can see in the definition of the get_parent linked few examples above, this method uses the get_block method from the runtime. Therefore, if we access the static_asset_path field in the prepare_runtime_for_user function, the parents (generated because of the InheritanceMixin) will not have the user data bound to them because the access operation happens before we define the get_block_for_descriptor method for the current runtime. If we do it later (e.g., in the ReplaceURLService.replace_urls method or the enclosing_sequence_for_gating_checks function), the get_block_for_descriptor method will already be in the runtime. Then, we will implicitly run the prepare_runtime_for_user function for these parent XBlocks, too (because of how the get_block works in x_module.py).

This comment is already loaded with information, so I won't dig into why this specific test failed - it was just an example where we could observe this behavior.

Part 2: Q/A

Also, can you clarify exactly what "until we remove all XBlock-specific code from here" means?

"The goal of FC-0026 is to remove the XBlock-specific handling from the prepare_runtime_for_user function." Currently, the order of runtime binding can affect the arguments passed to services in XBlocks we are initializing through an existing runtime. This is what was happening in #32165. Once we remove all the code that pulls data from a specific XBlock, the binding order won't matter anymore. That's why we're getting all data from the XBlocks directly within services. If a service uses some attributes of an XBlock, we're not initializing it in the prepare_runtime_for_user - we only create a partial, so the service is initialized only when it's used. It's invoked from the XBlock's code, so we can pass self (the currently used XBlock) to the partial. And this way, we can share these services between all XBlocks within the same course.

So we don't want the parent XBlock's runtimes to be bound to user-specific data?

This is the part I'm still investigating. That's why I wanted to retain the current behavior in this PR. In the mentioned test - yes, we don't want to bind it because it causes some side effects. However, once there is no XBlock-specific code in the prepare_runtime_for_user function, it should not be a huge difference anymore.
I was looking into all existing uses of the get_parent method and didn't observe any regressions after adding an experimental bind=False for the get_block invocation from there. Still, it's too early to say this for sure.

Part 3: Follow-up

By the way, it seems like we're now initializing a ton of services for every XBlock, regardless of whether or not they actually get used. I know the services are pretty lightweight but it might be good in the future to check block.service_declaration(...) before initializing services, or initialize them on demand like I did in the new runtime.

As a follow-up to my explanation from the last part, I have a WIP PR (#32420) with service caching. Only the last two commits are relevant - others are pulled from existing PRs to check the complete solution. In short - we're using the RequestCache to cache services for all runtimes with identical (user_id, course_id) pairs.

[bradenmacdonald]

Thanks for the very detailed response @Agrendalath. 👍🏻

I think we need to eventually get rid of having "bound" or "unbound" blocks within a runtime. The runtime should either be initialized with a user ID or without. If with a user ID, then regardless of what API is used, blocks should always be "bound" and have block.scope_ids.user_id set, whereas if the runtime is not initialized with a user ID then blocks should always be "unbound" and block.scope_ids.user_id should be None. It's way too confusing having unbound and bound blocks/descriptors within the same runtime environment. I know this would probably require some changes to LTI though.

[Agrendalath]

@bradenmacdonald, I agree - removing this distinction is a logical next step. Technically, we could remove all user-specific (and even course-specific) handling from the runtime services and pull this information directly from XBlocks during the service initialization. Once done, changes to the LTI and Instructor Dashboard tasks (that use a workaround to access the runtime) should be minimal. Some more work will be required to handle access, expirations, and masquerading, though.
However, this is outside the scope of FC-0026, which we should complete by the end of this month. I'll add this to the proposed scope of the next iteration of this project.

Is there anything left in this PR before we can call it ready?

[Agrendalath]

@ormsbee, would you like to review this before we schedule the merge?

[openedx-webhooks]

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX production environment.

[edx-pipeline-bot]

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.