Fix review comments

src/adapter/http/src/data/account_handler.rs

@@ -35,22 +35,20 @@ use opendatafabric::{AccountID, AccountName};
 
 #[derive(Debug, serde::Serialize)]
 #[serde(rename_all = "camelCase")]
-pub struct Response {
+pub struct AccountResponse {
     pub id: AccountID,
     pub account_name: AccountName,
     pub email: Option<String>,
     pub display_name: AccountDisplayName,
     pub account_type: AccountType,
     pub avatar_url: Option<String>,
     pub registered_at: DateTime<Utc>,
-    // TODO: ReBAC: absorb the `is_admin` attribute from the Accounts domain
-    //       https://github.com/kamu-data/kamu-cli/issues/766
     pub is_admin: bool,
     pub provider: String,
     pub provider_identity_key: String,
 }
 
-impl From<Account> for Response {
+impl From<Account> for AccountResponse {
     fn from(value: Account) -> Self {
         Self {
             id: value.id,
@@ -72,13 +70,13 @@ impl From<Account> for Response {
 #[transactional_handler]
 pub async fn account_handler(
     Extension(catalog): Extension<Catalog>,
-) -> Result<Json<Response>, ApiError> {
+) -> Result<Json<AccountResponse>, ApiError> {
     let response = get_account(&catalog).await?;
     tracing::debug!(?response, "Get account info response");
     Ok(response)
 }
 
-async fn get_account(catalog: &Catalog) -> Result<Json<Response>, ApiError> {
+async fn get_account(catalog: &Catalog) -> Result<Json<AccountResponse>, ApiError> {
     let current_account_subject = catalog.get_one::<CurrentAccountSubject>().unwrap();
     match current_account_subject.as_ref() {
         CurrentAccountSubject::Anonymous(_) => Err(ApiError::new_unauthorized()),

src/adapter/http/src/data/dataset_info_handler.rs

@@ -22,34 +22,44 @@ use database_common_macros::transactional_handler;
 use dill::Catalog;
 use http_common::*;
 use kamu_core::{DatasetRepository, GetDatasetError};
-use opendatafabric::{AccountName, DatasetID, DatasetName};
+use opendatafabric::{AccountName, DatasetHandle, DatasetID, DatasetName};
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
 #[derive(Debug, serde::Serialize)]
 #[serde(rename_all = "camelCase")]
-pub struct Response {
+pub struct DatasetInfoResponse {
     pub id: DatasetID,
     pub account_name: Option<AccountName>,
     pub dataset_name: DatasetName,
 }
 
+impl From<DatasetHandle> for DatasetInfoResponse {
+    fn from(value: DatasetHandle) -> Self {
+        Self {
+            id: value.id,
+            account_name: value.alias.account_name,
+            dataset_name: value.alias.dataset_name,
+        }
+    }
+}
+
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
 #[transactional_handler]
 pub async fn dataset_info_handler(
     Extension(catalog): Extension<Catalog>,
     Path(dataset_id): Path<DatasetID>,
-) -> Result<Json<Response>, ApiError> {
+) -> Result<Json<DatasetInfoResponse>, ApiError> {
     let response = get_dataset_by_id(&catalog, &dataset_id).await?;
-    tracing::debug!(?response, "Get workspace info response");
+    tracing::debug!(?response, "Get dataset by id info response");
     Ok(response)
 }
 
 async fn get_dataset_by_id(
     catalog: &Catalog,
     dataset_id: &DatasetID,
-) -> Result<Json<Response>, ApiError> {
+) -> Result<Json<DatasetInfoResponse>, ApiError> {
     let dataset_repo = catalog.get_one::<dyn DatasetRepository>().unwrap();
     let dataset_handle = dataset_repo
         .resolve_dataset_ref(&dataset_id.clone().as_local_ref())
@@ -59,11 +69,7 @@ async fn get_dataset_by_id(
             GetDatasetError::Internal(e) => e.api_err(),
         })?;
 
-    Ok(Json(Response {
-        id: dataset_handle.id,
-        account_name: dataset_handle.alias.account_name,
-        dataset_name: dataset_handle.alias.dataset_name,
-    }))
+    Ok(Json(dataset_handle.into()))
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

src/adapter/http/src/data/workspace_info_handler.rs

@@ -26,24 +26,24 @@ use kamu_core::DatasetRepository;
 
 #[derive(Debug, serde::Serialize)]
 #[serde(rename_all = "camelCase")]
-pub struct Response {
+pub struct WorkspaceInfoResponse {
     pub is_multi_tenant: bool,
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
 pub async fn workspace_info_handler(
     Extension(catalog): Extension<Catalog>,
-) -> Result<Json<Response>, ApiError> {
+) -> Result<Json<WorkspaceInfoResponse>, ApiError> {
     let response = get_workspace_info(&catalog);
     tracing::debug!(?response, "Get workspace info response");
     Ok(response)
 }
 
-fn get_workspace_info(catalog: &Catalog) -> Json<Response> {
+fn get_workspace_info(catalog: &Catalog) -> Json<WorkspaceInfoResponse> {
     let dataset_repo = catalog.get_one::<dyn DatasetRepository>().unwrap();
 
-    Json(Response {
+    Json(WorkspaceInfoResponse {
         is_multi_tenant: dataset_repo.is_multi_tenant(),
     })
 }

src/adapter/http/tests/tests/test_account_info.rs

@@ -80,7 +80,6 @@ struct AccountInfoHarness {
 
 impl AccountInfoHarness {
     async fn new(is_multi_tenant: bool) -> Self {
-        // TODO: Need access to these from harness level
         let run_info_dir = tempfile::tempdir().unwrap();
 
         let catalog = dill::CatalogBuilder::new()

src/app/cli/src/services/odf_server/access_token_registry_service.rs

@@ -69,7 +69,7 @@ impl AccessTokenRegistryService {
             let registry = registry_ptr
                 .lock()
                 .expect("Could not lock access tokens registry");
-            if let Some(server_record) = registry.iter().find(|c| {
+            let server_record_maybe = registry.iter().find(|c| {
                 if &c.backend_url == odf_server_url {
                     true
                 } else {
@@ -78,7 +78,8 @@ impl AccessTokenRegistryService {
                         _ => false,
                     }
                 }
-            }) {
+            });
+            if let Some(server_record) = server_record_maybe {
                 return server_record
                     .token_for_account(self.account_name())
                     .map(|ac| AccessTokenFindReport {
@@ -100,10 +101,11 @@ impl AccessTokenRegistryService {
                 .lock()
                 .expect("Could not lock access tokens registry");
 
-            if let Some(server_record) = registry.iter().find(|c| match &c.frontend_url {
+            let server_record_maybe = registry.iter().find(|c| match &c.frontend_url {
                 Some(frontend_url) => frontend_url == odf_server_frontend_url,
                 _ => false,
-            }) {
+            });
+            if let Some(server_record) = server_record_maybe {
                 return server_record
                     .token_for_account(self.account_name())
                     .map(|ac| AccessTokenFindReport {

src/infra/core/src/push_service_impl.rs

@@ -189,7 +189,7 @@ impl PushService for PushServiceImpl {
             for request in &initial_requests {
                 if let PushRequest {
                     local_ref: Some(local_ref),
-                    remote_ref: Some(_transfer_ref @ TransferDatasetRef::RemoteRef(remote_ref)),
+                    remote_ref: Some(TransferDatasetRef::RemoteRef(remote_ref)),
                 } = request
                 {
                     if let DatasetRefRemote::Alias(_) = &remote_ref {

src/infra/core/src/remote_alias_resolver_impl.rs

@@ -56,11 +56,11 @@ impl RemoteAliasResolverImpl {
             .await
             .int_err()?;
 
-        let mut push_aliases: Vec<_> = remote_aliases.get_by_kind(remote_alias_kind).collect();
+        let push_aliases: Vec<_> = remote_aliases.get_by_kind(remote_alias_kind).collect();
 
         match push_aliases.len() {
             0 => Ok(None),
-            1 => Ok(Some(push_aliases.remove(0).clone())),
+            1 => Ok(Some(push_aliases[0].clone())),
             _ => Err(ResolveAliasError::AmbiguousAlias),
         }
     }
@@ -72,10 +72,13 @@ impl RemoteAliasResolverImpl {
         dataset_name: &odf::DatasetName,
     ) -> Result<odf::DatasetRefRemote, InternalError> {
         let mut res_url = repo_url.clone().as_odf_protocol().int_err()?;
-        if let Some(account_name) = account_name_maybe {
-            res_url.path_segments_mut().unwrap().push(&account_name);
+        {
+            let mut path_segments = res_url.path_segments_mut().unwrap();
+            if let Some(account_name) = account_name_maybe {
+                path_segments.push(&account_name);
+            }
+            path_segments.push(dataset_name);
         }
-        res_url.path_segments_mut().unwrap().push(dataset_name);
         Ok(res_url.into())
     }
 
@@ -115,9 +118,9 @@ impl RemoteAliasResolver for RemoteAliasResolverImpl {
 
         if let Some(transfer_dataset_ref) = &transfer_dataset_ref_maybe {
             match transfer_dataset_ref {
-                odf::TransferDatasetRef::RemoteRef(
-                    _dataset_ref_remote @ DatasetRefRemote::Alias(dataset_alias_remote),
-                ) => {
+                odf::TransferDatasetRef::RemoteRef(DatasetRefRemote::Alias(
+                    dataset_alias_remote,
+                )) => {
                     repo_name = dataset_alias_remote.repo_name.clone();
                     account_name.clone_from(&dataset_alias_remote.account_name);
                     dataset_name = Some(dataset_alias_remote.dataset_name.clone());