Import linter improvements: enforce usage of a package's public API

[bradenmacdonald]

Description

#31062 introduced an import linter, with a very simple configuration to flag imports from LMS code into CMS and vice versa.

This PR extends that import linter in two ways, which are intended to help enforce clean boundaries and stronger API contracts among the packages that comprise edx-platform.

Improved detection of LMS<->CMS imports

First, the existing "lms-cms independence" contract has been expanded to include the openedx package namespace as well, which has found 14 additional existing imports (mostly from cms to lms) that shouldn't be there.

A typical example of CMS code importing LMS code is:

cms.djangoapps.export_course_metadata.tasks
[imports from:] -> openedx.core.djangoapps.schedules.content_highlights
[imports from:] -> lms.djangoapps.courseware.block_render

Enforce usage of a package's python API

Second, I have implemented a new linter that can be given package names like this:

isolated_apps =
    openedx.core.djangoapps.agreements
    openedx.core.djangoapps.bookmarks
    openedx.core.djangoapps.content_libraries
    openedx.core.djangoapps.olx_rest_api
    openedx.core.djangoapps.xblock

The idea is that these packages define their public API in api.py files (see OEP-49 Django App Patterns), so importing from any other module within those packages is an error (or in this case, a lint failure).

Using this initial small set of example packages, here are the issues I found:

Do not depend on non-public API of isolated apps.
-------------------------------------------------

openedx.core.djangolib.tests.test_blockstore_cache:8: imported from non-public API of openedx.core.djangoapps.content_libraries:
    from openedx.core.djangoapps.content_libraries.tests.base import (

openedx.core.lib.blockstore_api.tests.test_blockstore_api:11: imported from non-public API of openedx.core.djangoapps.content_libraries:
    from openedx.core.djangoapps.content_libraries.tests.base import (

lms.djangoapps.courseware.block_render:72: imported from non-public API of openedx.core.djangoapps.bookmarks:
    from openedx.core.djangoapps.bookmarks.services import BookmarksService

openedx.core.djangoapps.content_libraries.api:95: imported from non-public API of openedx.core.djangoapps.olx_rest_api:
    from openedx.core.djangoapps.olx_rest_api.block_serializer import XBlockSerializer

openedx.core.djangoapps.content_libraries.library_bundle:13: imported from non-public API of openedx.core.djangoapps.xblock:
    from openedx.core.djangoapps.xblock.runtime.blockstore_runtime import xml_for_definition

openedx.core.djangoapps.content_libraries.library_context:16: imported from non-public API of openedx.core.djangoapps.xblock:
    from openedx.core.djangoapps.xblock.learning_context import LearningContext

openedx.core.djangoapps.content_libraries.library_bundle:14: imported from non-public API of openedx.core.djangoapps.xblock:
    from openedx.core.djangoapps.xblock.runtime.olx_parsing import (

openedx.core.djangoapps.content_libraries.api:98: imported from non-public API of openedx.core.djangoapps.xblock:
    from openedx.core.djangoapps.xblock.runtime.olx_parsing import XBlockInclude

openedx.core.djangoapps.content_libraries.api:97: imported from non-public API of openedx.core.djangoapps.xblock:
    from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl

The first two items are a very nice example of catching issues: the code in question is not specific to content libraries but was living in the content_libraries app. To fix the issue, I moved it to the blockstore_api app which is a more relevant location.

I fixed all of the other errors found by exposing relevant code through api.py and updating imports to use that.

My hope is that over time we can grow this list, and enforce a clear API separation for packages that are designed that way.

I'm looking for feedback on this approach!

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

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.

[rgraber]

Can you explain what

cms.djangoapps.export_course_metadata.tasks
-> openedx.core.djangoapps.schedules.content_highlights
-> lms.djangoapps.courseware.block_render

means? is task importing from content_highlights which imports from block_render?

[bradenmacdonald]

is task importing from content_highlights which imports from block_render?

Yes, that's exactly what it means.

/cms/djangoapps/export_course_metadata/tasks imports from content_highlights which imports from lms.djangoapps.courseware.block_render.

Although now that I look those links, the LMS import in content_highlights is actually in a function and is not a top-level import, so it may or may not actually be used from cms. But it is using some low-level LMS code and there's probably a cleaner way to do what it's trying to do.

[robrap]

@bradenmacdonald: This seems really nice.

• There are so many places where we could use linting to better enforce our OEPs or ADRs, and this is a great example.
• I could imagine a related lint check that ensures apps have an api.py. I'm not certain that all do, but I guess any with external imports should.
• As with all our linting rules, would we benefit from adding for everything with a round of amnestied linting exceptions, to ensure that the problem won't grow? It would also be nice if we could point to docs about how to document any new lint exception, for example, by forcing developers to add a comment like: `# I acknowledge that I am adding technical debt.". (I'm only partially joking.)
  None of these thoughts block this idea or PR.

[bradenmacdonald]

As with all our linting rules, would we benefit from adding for everything with a round of amnestied linting exceptions, to ensure that the problem won't grow?

I would like to do that but I'm afraid the list of exceptions would be too long. Without having something like this in place, it's kind of been the wild west for cross-package imports. That said, I have only experimented with a few packages so far. It would probably be worth methodically testing every major package (at least in openedx.core) and seeing how close they are to passing this lint rule. There may be others like openedx.core.djangoapps.agreements which was already passing this rule without any changes; I haven't tested that many. But I definitely found some that have dozens and dozens of violations.

[bradenmacdonald]

Following up on my previous comment: Currently this is opt-in on a per-package basis. I definitely could change it to an opt-out approach: default to linting all packages in certain prefixes like openedx.core.djangoapps.*, and grandfather some existing problematic ones in by opting them out.

I believe this will also support linting openedx django app packages that are developed in separate repos but installed into our edxapp venv, too, but I haven't tried that yet. (For example, it could check that we don't use anything from the enterprise package except enterprise.api.*, even though enterprise is from a separate repo, since it's still installed at the time this linter is run.)

[robrap]

I would like to do that but I'm afraid the list of exceptions would be too long.

If the exceptions are all automatically marked with a specially designated amnesty disable pragma, why does the quantity matter? They exist either way, and it seems better to lock things down so no more are added. Anyway, just a thought.

[bradenmacdonald]

If the exceptions are all automatically marked with a specially designated amnesty disable pragma, why does the quantity matter?

Currently they'd all have to be listed in the setup.cfg file, but I suppose I could modify the new linter's code to detect an inline pragma in the code, and also write a script to auto-add the pragma. So yeah that is an option, just something that needs a bit more work to implement.

[mphilbrick211]

Hi @bradenmacdonald and @robrap! Just checking in on this :)

[bradenmacdonald]

@mphilbrick211 I was just waiting for more feedback on this... @ormsbee what did you think?

[ormsbee]

This is great stuff, and I totally agree with opt-in to start. It might be worth adding something to OEP-49 after this merges.

My only question has to do with why it was necessary to create the api_impl.py module and move things around.

[bradenmacdonald]

@ormsbee

My only question has to do with why it was necessary to create the api_impl.py module and move things around.

Basically there was a circular import. I wanted to expose BookmarkService (from .services import BookmarksService) in api.py but its implementation in services.py requires code from api.py. It caused a circular import error unless I moved the API implementation into a separate file like this.

That was the primary reason, and the secondary reason is that the code in api.py was importing a number of symbols that shouldn't be part of the public API (DEFAULT_FIELDS, OPTIONAL_FIELDS, Bookmark, BookmarkSerializer, modulestore, ItemNotFoundError, settings, tracker) and it's much easier to make them private this way (importing them into api_impl.py without changes) than it is to import them with an underscore prefix and change the code to use the prefixed imports.

[robrap]

@bradenmacdonald: Consider adding a little of that context to the docstring for api_impl.py so this is more obvious in the future.

[ormsbee]

(Well, approved once that last test suite passes.)

[openedx-webhooks]

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

[bradenmacdonald]

Thanks for the feedback everyone! I have left this as opt-in for now, and I've opened another PR to update the OEP to mention this, if someone would like to review: openedx/openedx-proposals#467

[edx-pipeline-bot]

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

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.