Fix: Mirror URI in manifest /index/files response is set for MA files (#7687) #7693
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
7 times, most recently
from
52a4ca7 to
d8627ae
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #7693 +/- ##
===========================================
+ Coverage 84.82% 84.86% +0.04%
===========================================
Files 157 157
Lines 23060 23141 +81
===========================================
+ Hits 19561 19639 +78
- Misses 3499 3502 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
3 times, most recently
from
39b3cd7 to
87d7dea
Compare
hannes-ucsc
requested changes
Jan 20, 2026
src/azul/plugins/__init__.py
Outdated
Comment on lines
665
to
666
| Implementations should raise PermissionError if the provided | ||
| authentication is insufficient to access the repository. |
Member
There was a problem hiding this comment.
"Implementations" implies "implementations of this method". No authentication is provided to those implementations, so I don't understand this sentence.
src/azul/service/source_service.py
Outdated
Comment on lines
138
to
139
| all_sources = set() | ||
| public_sources = set() |
Member
There was a problem hiding this comment.
We typically use tuple assignmets when they correlate like this.
src/azul/service/source_service.py
Outdated
| return int(time()) | ||
|
|
||
| @cache | ||
| def _configured_sources(self) -> Mapping[str, AbstractSet[SourceRef]]: |
Member
There was a problem hiding this comment.
The return type hint is a text-book case for a typed dict.
hannes-ucsc
requested changes
Jan 20, 2026
Member
There was a problem hiding this comment.
Also, please add unit test coverage for the the newly uncovered lines
https://app.codecov.io/gh/DataBiosphere/azul/pull/7693/blob/src/azul/service/source_service.py
Contributor
Author
It wasn't caught because |
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
from
87d7dea to
7464fca
Compare
| def _now(self) -> int: | ||
| return int(time()) | ||
|
|
||
| @cache |
Check warning
Code scanning / CodeQL
Use of the return value of a procedure Warning
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
from
7464fca to
73ab983
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
6 times, most recently
from
f3a06d6 to
4b96407
Compare
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7687-mirror-uri-set-for-ma-files
branch
from
4b96407 to
63349fb
Compare
hannes-ucsc
requested changes
Jan 30, 2026
Member
hannes-ucsc
left a comment
hannes-ucsc
left a comment
There was a problem hiding this comment.
Index: src/azul/plugins/__init__.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/plugins/__init__.py b/src/azul/plugins/__init__.py
--- a/src/azul/plugins/__init__.py (revision 7af580537acff684454d834f8e7f597cb3aea48e)
+++ b/src/azul/plugins/__init__.py (date 1769790105893)
@@ -658,8 +658,14 @@
) -> list[SOURCE_REF]:
"""
Filter the given sources to only include sources that the plugin is
- configured to read metadata from, and instantiate them as `SourceRef`s.
+ configured to read metadata from, and return a ``SourceRef`` instance
+ for each matching source.
"""
+
+ # REVIEW: Please assert the uniqueness constraint on `name` here by
+ # ensuring that no entries are overwritten in either dictionary
+ # comprehensions
+
configured_specs_by_name = {spec.name: spec for spec in self.sources}
source_ids_by_name = {
name: id
Index: src/azul/service/source_service.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/source_service.py b/src/azul/service/source_service.py
--- a/src/azul/service/source_service.py (revision 7af580537acff684454d834f8e7f597cb3aea48e)
+++ b/src/azul/service/source_service.py (date 1769791839845)
@@ -13,6 +13,7 @@
CatalogName,
NotInLambdaContextException,
cache,
+ cached_property,
config,
open_resource,
)
@@ -54,8 +55,8 @@
class _ConfiguredSources(TypedDict):
- all: AbstractSet[SourceRef]
- public: AbstractSet[SourceRef]
+ sources: AbstractSet[SourceRef]
+ public_sources: AbstractSet[SourceRef]
class SourceService:
@@ -64,6 +65,10 @@
def repository_plugin(self, catalog: CatalogName) -> RepositoryPlugin:
return RepositoryPlugin.load(catalog).create(catalog)
+ # REVIEW: Please provide docstrings for the public methods, with a focus on
+ # how they differ from each other, especially with respect to
+ # whether and for how long they are being cached.
+
def list_accessible_source_ids(self,
catalog: CatalogName,
authentication: Authentication | None
@@ -90,6 +95,9 @@
) -> Iterable[SourceRef]:
return self.repository_plugin(catalog).list_accessible_sources(authentication)
+ def list_sources(self, catalog_name: str) -> Iterable[SourceRef]:
+ return self.repository_plugin(catalog_name).list_sources()
+
table_name = config.dynamo_sources_cache_table_name
key_attribute = 'identity'
@@ -133,45 +141,46 @@
def _now(self) -> int:
return int(time())
- @cache
+ @cached_property
def _configured_sources(self) -> _ConfiguredSources:
try:
with open_resource('sources.json') as f:
sources = json.load(f)
except NotInLambdaContextException:
- all_sources, public_sources = set(), set()
+ sources, public_sources = set(), set()
for catalog in config.catalogs.values():
if not catalog.is_integration_test_catalog:
- all_sources.update(self.repository_plugin(catalog.name).list_sources())
+ catalog_name = catalog.name
+ sources.update(self.list_sources(catalog_name))
public_sources.update(self.list_accessible_sources(catalog.name,
authentication=None))
return {
- 'all': all_sources,
- 'public': public_sources,
+ 'sources': sources,
+ 'public_sources': public_sources,
}
else:
- def parse(sources: AnyJSON) -> AbstractSet[SourceRef]:
+ def from_json(sources: AnyJSON) -> AbstractSet[SourceRef]:
return frozenset(
SourceRef.from_json(source)
for source in json_element_mappings(sources)
)
return {
- 'all': parse(sources['all']),
- 'public': parse(sources['public']),
+ 'sources': from_json(sources['sources']),
+ 'public_sources': from_json(sources['public_sources']),
}
@property
def configured_sources(self) -> AbstractSet[SourceRef]:
- return self._configured_sources()['all']
+ return self._configured_sources['sources']
@property
def configured_public_sources(self) -> AbstractSet[SourceRef]:
- return self._configured_sources()['public']
+ return self._configured_sources['public_sources']
@property
def configured_sources_for_outsourcing(self) -> JSON:
return {
k: [source.to_json() for source in v]
- for k, v in self._configured_sources().items()
+ for k, v in self._configured_sources.items()
}
Index: src/azul/indexer/__init__.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/indexer/__init__.py b/src/azul/indexer/__init__.py
--- a/src/azul/indexer/__init__.py (revision 7af580537acff684454d834f8e7f597cb3aea48e)
+++ b/src/azul/indexer/__init__.py (date 1769791839840)
@@ -423,7 +423,8 @@
are structured might want to implement this abstract class. Plugins that
have simple unstructured names may want to use :class:`SimpleSourceSpec`.
"""
- #: Assumed to be unique per catalog.
+
+ #: The name of this source. Azul assumes this to be unique per catalog.
name: str
@@ -466,9 +467,11 @@
Note to plugin implementers: Since the source ID can't be assumed to be
globally unique, plugins should subclass this class, even if the subclass
- body is empty. Additionally, subclasses must not add any fields that are
- required by the constructor, since the base repository plugin needs to be
- able to instantiate them generically.
+ body is empty. Additionally subclasses may not override the constructor in a
+ way that changes its signature, to allow for polymorphic instantiation.
+ Specifically, this means that subclasses may not add fields without a
+ default, provide a default for an inherited field, or modify whether a field
+ is initialized via a keyword or positional argument.
>>> spec = SimpleSourceSpec(name='')
>>> prefix = Prefix(partition=0)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Linked issues: #7687
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all linked issuespartiallabelAuthor (reindex)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAuthor (API changes)
APIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make docker_images.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifydocker_images.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues linked to this PRAuthor (before every review)
develop, squashed fixups from prior reviewsmake requirements_updateor this PR does not modifyDockerfile,environment,requirements*.txt,common.mk,Makefileorenvironment.bootRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
Note that after requesting changes, the PR must be assigned to only the author.
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator
reindex:…labels andrcommit title tagno demodevelopOperator (deploy
.sharedand.gitlabcomponents)_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator (post-deploy of
.gitlabcomponent)dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (deploy runner image)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnerOperator (sandbox build)
sandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices insandboxanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvilboxsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevOperator (merge the branch)
pif the PR is also labeledpartialOperator (main build)
devanvildevdevdevanvildevanvildev_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevdevor this PR does not require reindexingdevdeploy_browserjob in the GitLab pipeline for this PR indevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdeploy_browserjob in the GitLab pipeline for this PR inanvildevor this PR does not require reindexinganvildevOperator (mirroring)
devor this PR does not require mirroringdevanvildevor this PR does not require mirroringanvildevdevor this PR does not require mirroringdevanvildevor this PR does not require mirroringanvildevdevor this PR does not require mirroringdevanvildevor this PR does not require mirroringanvildevOperator
deploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem