Simplify permission checks for creating namespaces #1214
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
Reviewer's GuideIntroduces a centralized Catalog.is_cli flag to consolidate environment context, removes deprecated metastore permission properties, refactors CLI commands and loader to route between local and Studio operations based on is_cli, updates save and creation workflows to enforce CLI restrictions, and aligns tests with the new mechanism. Sequence diagram for dataset removal with new is_cli logicsequenceDiagram
actor User
participant CLI
participant Catalog
participant Config
participant Studio
participant Metastore
User->>CLI: rm_dataset(...)
CLI->>Catalog: get_full_dataset_name(name)
CLI->>Catalog: is_cli
alt is_cli and studio
CLI->>Config: read studio token
alt token exists
CLI->>Studio: remove_studio_dataset(...)
else token missing
CLI->>CLI: raise DataChainError
end
else
CLI->>Metastore: get_project(...)
CLI->>Catalog: edit local dataset
end
Class diagram for Catalog and Metastore permission refactorclassDiagram
class Catalog {
- _is_cli: bool
+ is_cli: bool
}
class AbstractMetastore {
<<abstract>>
- Removed: is_studio: bool
- Removed: is_local_dataset(dataset_namespace: str): bool
- Removed: namespace_allowed_to_create: bool
- Removed: project_allowed_to_create: bool
}
Catalog --> AbstractMetastore : metastore
class SQLiteMetastore {
// No longer implements is_studio
}
AbstractMetastore <|-- SQLiteMetastore
Class diagram for loader and Catalog instantiation changesclassDiagram
class Loader {
+ get_catalog(...): Catalog
}
class Catalog {
+ is_cli: bool
}
Loader --> Catalog : returns
class SQLiteMetastore
Loader ..> SQLiteMetastore : uses for is_cli detection
Class diagram for namespace and project creation permission checksclassDiagram
class Session {
+ catalog: Catalog
}
class Catalog {
+ is_cli: bool
}
class Namespace {
+ validate_name(name)
}
class Project {
+ validate_name(name)
}
Session --> Catalog
Namespace ..> Session : uses
Project ..> Session : uses
Namespace ..> Catalog : checks is_cli for permission
Project ..> Catalog : checks is_cli for permission
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ilongin - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/datachain/lib/namespaces.py:31` </location>
<code_context>
"""
session = Session.get(session)
- if not session.catalog.metastore.namespace_allowed_to_create:
+ if session.catalog.is_cli:
raise NamespaceCreateNotAllowedError("Creating namespace is not allowed")
</code_context>
<issue_to_address>
Logic for namespace creation restriction appears inverted.
This change may block namespace creation in CLI mode even when permitted. Please verify that this aligns with the intended behavior for both CLI and Studio environments.
</issue_to_address>
### Comment 2
<location> `src/datachain/lib/projects.py:35` </location>
<code_context>
"""
session = Session.get(session)
- if not session.catalog.metastore.project_allowed_to_create:
+ if session.catalog.is_cli:
raise ProjectCreateNotAllowedError("Creating project is not allowed")
</code_context>
<issue_to_address>
Project creation restriction logic may be reversed.
The updated condition blocks project creation in CLI mode, which may not be intended. Confirm that this matches the desired permission logic for CLI and Studio environments.
</issue_to_address>
### Comment 3
<location> `src/datachain/lib/dc/datasets.py:346` </location>
<code_context>
):
namespace_name, project_name, name = catalog.get_full_dataset_name(name)
- if not catalog.metastore.is_local_dataset(namespace_name) and studio:
+ if catalog.is_cli and studio:
+ # removing Studio dataset from CLI
from datachain.studio import remove_studio_dataset
</code_context>
<issue_to_address>
Dataset deletion logic now depends on CLI mode rather than dataset locality.
Please verify that this change aligns with the intended behavior and that all relevant scenarios are covered.
</issue_to_address>
### Comment 4
<location> `src/datachain/catalog/catalog.py:530` </location>
<code_context>
Callable[["AbstractWarehouse"], None]
] = None,
in_memory: bool = False,
+ is_cli: Optional[bool] = True,
):
datachain_dir = DataChainDir(cache=cache_dir, tmp=tmp_dir)
</code_context>
<issue_to_address>
Defaulting is_cli to True may not always reflect the actual environment.
This could cause incorrect behavior if Catalog is used outside a CLI context. Recommend setting is_cli explicitly where Catalog is instantiated or inferring it from the metastore type.
Suggested implementation:
```python
in_memory: bool = False,
is_cli: Optional[bool] = None,
):
datachain_dir = DataChainDir(cache=cache_dir, tmp=tmp_dir)
datachain_dir.init()
}
self._warehouse_ready_callback = warehouse_ready_callback
self.in_memory = in_memory
# Infer is_cli from metastore type if not explicitly provided
if is_cli is not None:
self._is_cli = is_cli
else:
# Example inference: if metastore is a CLI metastore, set True, else False
# You may need to adjust this logic based on your actual metastore types
self._is_cli = hasattr(self.metastore, "is_cli") and getattr(self.metastore, "is_cli", False)
```
- You will need to ensure that all instantiations of this class in your codebase are updated to explicitly pass `is_cli` if inference is not desired or not possible.
- Adjust the inference logic as needed to match your actual metastore implementation and types.
</issue_to_address>
### Comment 5
<location> `src/datachain/catalog/loader.py:140` </location>
<code_context>
we can provide DATACHAIN_METASTORE_ARG_TEAM_ID=12345 env variable.
"""
from datachain.catalog import Catalog
+ from datachain.data_storage.sqlite import SQLiteMetastore
+ metastore = get_metastore(in_memory=in_memory)
return Catalog(
- metastore=get_metastore(in_memory=in_memory),
+ metastore=metastore,
warehouse=get_warehouse(in_memory=in_memory),
client_config=client_config,
in_memory=in_memory,
+ is_cli=isinstance(metastore, SQLiteMetastore),
)
</code_context>
<issue_to_address>
CLI mode is inferred from metastore type, which may not be robust.
Relying on isinstance for CLI mode detection may break if new metastore types are added or if SQLiteMetastore is used outside CLI. Please use a more explicit check for CLI mode.
Suggested implementation:
```python
metastore = get_metastore(in_memory=in_memory)
# Explicitly determine CLI mode, e.g., via a function argument or environment variable
is_cli = bool(os.environ.get("DATACHAIN_CLI_MODE", False))
return Catalog(
metastore=metastore,
warehouse=get_warehouse(in_memory=in_memory),
client_config=client_config,
in_memory=in_memory,
is_cli=is_cli,
)
```
- You will need to import `os` at the top of the file if it is not already imported.
- If CLI mode should be determined differently (e.g., via a function argument), adjust the assignment to `is_cli` accordingly and update the function signature and all call sites.
</issue_to_address>
### Comment 6
<location> `src/datachain/lib/dc/datachain.py:585` </location>
<code_context>
project = self.session.catalog.metastore.get_project(
project_name,
namespace_name,
- create=self.session.catalog.metastore.project_allowed_to_create,
+ create=not self.session.catalog.is_cli,
)
except ProjectNotFoundError as e:
</code_context>
<issue_to_address>
Project creation flag is now inverted based on CLI mode.
This change may prevent project creation in CLI mode, which differs from the previous behavior. Please verify if this aligns with the intended permissions.
</issue_to_address>
### Comment 7
<location> `tests/unit/lib/test_namespace.py:29` </location>
<code_context>
[email protected]("allow_create_namespace", [False])
[email protected]("is_cli", [True])
@skip_if_not_sqlite
-def test_create_by_user_not_allowed(test_session, allow_create_namespace):
+def test_create_by_user_not_allowed(test_session, is_cli):
with pytest.raises(NamespaceCreateNotAllowedError) as excinfo:
create_namespace("dev", session=test_session)
</code_context>
<issue_to_address>
Test for namespace creation denial is preserved and updated.
Consider adding a test for when 'is_cli' is False to verify that namespace creation is permitted in that case.
</issue_to_address>
### Comment 8
<location> `tests/unit/lib/test_project.py:65` </location>
<code_context>
)
[email protected]("allow_create_project", [False])
[email protected]("is_cli", [True])
@skip_if_not_sqlite
-def test_save_create_project_not_allowed(test_session, allow_create_project):
</code_context>
<issue_to_address>
Test for project creation denial updated to use 'is_cli'.
Please also add a test for when 'is_cli' is False to confirm project creation is allowed in that scenario.
</issue_to_address>
### Comment 9
<location> `tests/unit/lib/test_datachain.py:3591` </location>
<code_context>
)
[email protected]("allow_create_project", [False])
[email protected]("is_cli", [True])
@skip_if_not_sqlite
-def test_save_create_project_not_allowed(test_session, allow_create_project):
</code_context>
<issue_to_address>
Test for project creation not allowed updated to use 'is_cli'.
Please add a test for when 'is_cli' is False to ensure both allowed and not allowed cases are covered.
Suggested implementation:
```python
@pytest.mark.parametrize("is_cli", [True, False])
@skip_if_not_sqlite
def test_save_create_project_not_allowed(test_session, is_cli):
if is_cli:
with pytest.raises(ProjectCreateNotAllowedError):
dc.read_values(fib=[1, 1, 2, 3, 5, 8], session=test_session).save(
"dev.numbers.fibonacci"
)
else:
# Should succeed when project creation is allowed
result = dc.read_values(fib=[1, 1, 2, 3, 5, 8], session=test_session).save(
"dev.numbers.fibonacci"
)
assert result is not None
```
- Ensure that the `dc` object and the `save` method are correctly set up to respect the `is_cli` parameter in your actual implementation.
- Adjust the assertion for the allowed case (`is_cli=False`) if there is a more specific expected result than just `result is not None`.
</issue_to_address>
### Comment 10
<location> `tests/unit/lib/test_datachain.py:3226` </location>
<code_context>
@pytest.mark.parametrize("force", (True, False))
[email protected]("is_cli", (True,))
@skip_if_not_sqlite
def test_delete_dataset_from_studio(test_session, studio_token, requests_mock, force):
</code_context>
<issue_to_address>
Studio dataset deletion tests parameterized with 'is_cli'.
Please add tests for 'is_cli=False' to cover the non-Studio deletion path as well.
Suggested implementation:
```python
@pytest.mark.parametrize("force", (True, False))
@pytest.mark.parametrize("is_cli", (True, False))
@skip_if_not_sqlite
def test_delete_dataset_from_studio(test_session, studio_token, requests_mock, force):
```
```python
@pytest.mark.parametrize("is_cli", (True, False))
@skip_if_not_sqlite
def test_delete_dataset_from_studio_not_found(
test_session, studio_token, requests_mock
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -32,7 +32,7 @@ def create( | |||
| """ | |||
| session = Session.get(session) | |||
|
|
|||
| if not session.catalog.metastore.project_allowed_to_create: | |||
There was a problem hiding this comment.
issue (bug_risk): Project creation restriction logic may be reversed.
The updated condition blocks project creation in CLI mode, which may not be intended. Confirm that this matches the desired permission logic for CLI and Studio environments.
| @@ -527,6 +527,7 @@ def __init__( | |||
| Callable[["AbstractWarehouse"], None] | |||
| ] = None, | |||
| in_memory: bool = False, | |||
| is_cli: Optional[bool] = True, | |||
There was a problem hiding this comment.
suggestion (bug_risk): Defaulting is_cli to True may not always reflect the actual environment.
This could cause incorrect behavior if Catalog is used outside a CLI context. Recommend setting is_cli explicitly where Catalog is instantiated or inferring it from the metastore type.
Suggested implementation:
in_memory: bool = False,
is_cli: Optional[bool] = None,
):
datachain_dir = DataChainDir(cache=cache_dir, tmp=tmp_dir)
datachain_dir.init()
}
self._warehouse_ready_callback = warehouse_ready_callback
self.in_memory = in_memory
# Infer is_cli from metastore type if not explicitly provided
if is_cli is not None:
self._is_cli = is_cli
else:
# Example inference: if metastore is a CLI metastore, set True, else False
# You may need to adjust this logic based on your actual metastore types
self._is_cli = hasattr(self.metastore, "is_cli") and getattr(self.metastore, "is_cli", False)- You will need to ensure that all instantiations of this class in your codebase are updated to explicitly pass
is_cliif inference is not desired or not possible. - Adjust the inference logic as needed to match your actual metastore implementation and types.
src/datachain/catalog/loader.py
Outdated
Comment on lines
140
to
148
| from datachain.data_storage.sqlite import SQLiteMetastore | ||
|
|
||
| metastore = get_metastore(in_memory=in_memory) | ||
| return Catalog( | ||
| metastore=get_metastore(in_memory=in_memory), | ||
| metastore=metastore, | ||
| warehouse=get_warehouse(in_memory=in_memory), | ||
| client_config=client_config, | ||
| in_memory=in_memory, | ||
| is_cli=isinstance(metastore, SQLiteMetastore), |
There was a problem hiding this comment.
suggestion: CLI mode is inferred from metastore type, which may not be robust.
Relying on isinstance for CLI mode detection may break if new metastore types are added or if SQLiteMetastore is used outside CLI. Please use a more explicit check for CLI mode.
Suggested implementation:
metastore = get_metastore(in_memory=in_memory)
# Explicitly determine CLI mode, e.g., via a function argument or environment variable
is_cli = bool(os.environ.get("DATACHAIN_CLI_MODE", False))
return Catalog(
metastore=metastore,
warehouse=get_warehouse(in_memory=in_memory),
client_config=client_config,
in_memory=in_memory,
is_cli=is_cli,
)- You will need to import
osat the top of the file if it is not already imported. - If CLI mode should be determined differently (e.g., via a function argument), adjust the assignment to
is_cliaccordingly and update the function signature and all call sites.
| @skip_if_not_sqlite | ||
| def test_create_by_user_not_allowed(test_session, allow_create_namespace): |
There was a problem hiding this comment.
suggestion (testing): Test for namespace creation denial is preserved and updated.
Consider adding a test for when 'is_cli' is False to verify that namespace creation is permitted in that case.
| @@ -62,7 +62,7 @@ def test_invalid_name(test_session, name): | |||
|
|
|||
|
|
|||
| @skip_if_not_sqlite | |||
| @pytest.mark.parametrize("allow_create_project", [False]) | |||
There was a problem hiding this comment.
suggestion (testing): Test for project creation denial updated to use 'is_cli'.
Please also add a test for when 'is_cli' is False to confirm project creation is allowed in that scenario.
| @@ -3588,9 +3585,9 @@ def _full_name(namespace, project, name) -> str: | |||
| ) | |||
There was a problem hiding this comment.
suggestion (testing): Test for project creation not allowed updated to use 'is_cli'.
Please add a test for when 'is_cli' is False to ensure both allowed and not allowed cases are covered.
Suggested implementation:
@pytest.mark.parametrize("is_cli", [True, False])
@skip_if_not_sqlite
def test_save_create_project_not_allowed(test_session, is_cli):
if is_cli:
with pytest.raises(ProjectCreateNotAllowedError):
dc.read_values(fib=[1, 1, 2, 3, 5, 8], session=test_session).save(
"dev.numbers.fibonacci"
)
else:
# Should succeed when project creation is allowed
result = dc.read_values(fib=[1, 1, 2, 3, 5, 8], session=test_session).save(
"dev.numbers.fibonacci"
)
assert result is not None- Ensure that the
dcobject and thesavemethod are correctly set up to respect theis_cliparameter in your actual implementation. - Adjust the assertion for the allowed case (
is_cli=False) if there is a more specific expected result than justresult is not None.
Comment on lines
184
to
188
| token = Config().read().get("studio", {}).get("token") | ||
| if not token: | ||
| raise DataChainError( | ||
| "Not logged in to Studio. Log in with 'datachain auth login'." | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches)
Deploying datachain-documentation with
|
| Latest commit: |
87715c4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dd3b8263.datachain-documentation.pages.dev |
| Branch Preview URL: | https://ilongin-1208-simplify-permis.datachain-documentation.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
==========================================
- Coverage 88.74% 88.73% -0.01%
==========================================
Files 153 153
Lines 13848 13838 -10
Branches 1938 1938
==========================================
- Hits 12289 12279 -10
Misses 1103 1103
Partials 456 456
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey @ilongin - I've reviewed your changes - here's some feedback:
- Consider defaulting Catalog.is_cli to False (and overriding it only for CLI contexts in the loader) so that non-SQLite/metastore use-cases aren’t erroneously treated as CLI by default.
- The repeated pattern of checking for a studio token and raising a DataChainError in CLI commands could be extracted into a helper to reduce duplication and improve readability.
- Add a docstring or brief comment on the new is_cli property in Catalog to clearly document its intended semantics (CLI vs Studio) for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider defaulting Catalog.is_cli to False (and overriding it only for CLI contexts in the loader) so that non-SQLite/metastore use-cases aren’t erroneously treated as CLI by default.
- The repeated pattern of checking for a studio token and raising a DataChainError in CLI commands could be extracted into a helper to reduce duplication and improve readability.
- Add a docstring or brief comment on the new is_cli property in Catalog to clearly document its intended semantics (CLI vs Studio) for future maintainers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
shcheklein
reviewed
Jul 14, 2025
| @@ -527,6 +527,7 @@ def __init__( | |||
| Callable[["AbstractWarehouse"], None] | |||
| ] = None, | |||
| in_memory: bool = False, | |||
| is_cli: Optional[bool] = True, | |||
There was a problem hiding this comment.
just to double check is it really optional?
There was a problem hiding this comment.
so, it is really optional?
let's also come up with a better name please, is_cli is confusing ... you can do is_studio for example, False by default.
shcheklein
reviewed
Jul 14, 2025
src/datachain/catalog/catalog.py
Outdated
| @@ -1111,7 +1117,12 @@ def get_dataset_with_remote_fallback( | |||
| if version: | |||
| update = False | |||
|
|
|||
| if self.metastore.is_local_dataset(namespace_name) or not update: | |||
| # local dataset is the one that is in Studio or in CLI but has default namespace | |||
| is_local = ( | |||
There was a problem hiding this comment.
I'm not sure I understand this condition - how can we make it simpler, easier to read? (I'm also not sure I understand the comment above)
There was a problem hiding this comment.
Before namespaces / projects we didn't have any kind of check here and in Studio we would still try to fetch from remote (Studio) if missing locally, which was wrong (it would fail with strange error like missing token).
Now we can determine if dataset we are fetching is from it's own (local) DB or can be fetched from remote / Studio if missing locally.
We never fallback to Studio if:
- Script is already ran in Studio
- Dataset starts with
local.local.*
I agree this whole function is way to complex and confusing and I will try to refactor it.
There was a problem hiding this comment.
I've added better comment for that variable and changed name. I think it should be clear now.
shcheklein
reviewed
Jul 14, 2025
src/datachain/catalog/loader.py
Outdated
| warehouse=get_warehouse(in_memory=in_memory), | ||
| client_config=client_config, | ||
| in_memory=in_memory, | ||
| is_cli=isinstance(metastore, SQLiteMetastore), |
There was a problem hiding this comment.
there should be a better (explicit way) to set this up ... also, it means not only CLI but local Python, right? Why is it called CLI then?
There was a problem hiding this comment.
We usually call CLI for anything other than Studio so that's why I called it that way. It can be renamed.
I was weighing between what you mentioned, only set this explicitly, or have implicit (if explicit flag doesn't exist) determination as it's implemented now. If you have strong opinion only explicit is better I can do that way.
There was a problem hiding this comment.
Removed implicit logic and leaved only explicit arg
shcheklein
reviewed
Jul 14, 2025
| @@ -164,21 +167,22 @@ def edit_dataset( | |||
| ): | |||
| namespace_name, project_name, name = catalog.get_full_dataset_name(name) | |||
|
|
|||
| if catalog.metastore.is_local_dataset(namespace_name): | |||
| if catalog.is_cli and namespace_name != catalog.metastore.default_namespace_name: | |||
There was a problem hiding this comment.
also, not sure I understand (too complicated) .. what exactly we are detecting here?
There was a problem hiding this comment.
If someone does this: datachain ds edit dev.my-project.cats --new-name "dogs" this means it wants to edit Studio dataset and not local one. What's extra here, and should be removed is catalog.is_cli as by default this is called in CLI
There was a problem hiding this comment.
Removed catalog.is_cli condition
shcheklein
reviewed
Jul 17, 2025
| "Not logged in to Studio. Log in with 'datachain auth login'." | ||
| ) | ||
| else: | ||
| # if catalog.metastore.is_local_dataset(namespace_name): |
shcheklein
reviewed
Jul 17, 2025
| @@ -164,21 +167,22 @@ def edit_dataset( | |||
| ): | |||
| namespace_name, project_name, name = catalog.get_full_dataset_name(name) | |||
|
|
|||
| if catalog.metastore.is_local_dataset(namespace_name): | |||
| if namespace_name != catalog.metastore.default_namespace_name: | |||
There was a problem hiding this comment.
let's introduce a descriptive var - studio_dataset = ...
to make condisiton descriptive:
if studio_dataset:
....
There was a problem hiding this comment.
(tbh still don't a lot this very non obvious way to detect it by analyzing namespaces, comparing it with default - one needs to know a lot about namespaces to understand this code and why it is correct - it is not obvious)
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.
Trying to simplify logic around permissions like "is creation of namespace / project allowed or not" by lifting awareness of "is the process run in CLI or Studio" to
Catalogclass.This allowed to remove couple of methods from metastore:
is_studiois_local_datasetnamespace_allowed_to_createproject_allowed_to_createSummary by Sourcery
Simplify permission checks by replacing metastore-based flags with a centralized
Catalog.is_cliindicator, update CLI commands to use it for routing between local and Studio operations, refactor loader and save logic accordingly, and align tests with the new mechanism.Enhancements:
Catalog.is_cliflag and remove deprecated metastore properties (is_studio,is_local_dataset,namespace_allowed_to_create,project_allowed_to_create).rm_dataset,edit_dataset,delete_dataset) to usecatalog.is_clifor permission and routing logic.is_clibased on metastore type and propagate it through theCatalogconstructor.Tests:
allow_create_projectandallow_create_namespacefixtures with a singleis_clifixture and mockCatalog.is_cli.is_cliinstead of legacy allow-create flags.Summary by Sourcery
Centralize environment awareness by adding Catalog.is_cli and removing legacy metastore flags, refactor related dataset, namespace, and project commands to use the new flag, and align tests with the updated permission mechanism
Enhancements:
Tests: