Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add switch orga to legacy routes #8257

Merged
merged 17 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions app/controllers/DatasetController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import models.folder.FolderService
import models.organization.OrganizationDAO
import models.team.{TeamDAO, TeamService}
import models.user.{User, UserDAO, UserService}
import net.liftweb.common.{Failure, Full}
import net.liftweb.common.{Failure, Full, Empty}
import play.api.i18n.{Messages, MessagesProvider}
import play.api.libs.functional.syntax._
import play.api.libs.json._
Expand Down Expand Up @@ -85,6 +85,7 @@ class DatasetController @Inject()(userService: UserService,
thumbnailService: ThumbnailService,
thumbnailCachingService: ThumbnailCachingService,
conf: WkConf,
authenticationController: AuthenticationController,
analyticsService: AnalyticsService,
mailchimpClient: MailchimpClient,
wkExploreRemoteLayerService: WKExploreRemoteLayerService,
Expand Down Expand Up @@ -406,13 +407,30 @@ class DatasetController @Inject()(userService: UserService,
def getDatasetIdFromNameAndOrganization(datasetName: String, organizationId: String): Action[AnyContent] =
sil.UserAwareAction.async { implicit request =>
for {
dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId) ?~> notFoundMessage(datasetName) ~> NOT_FOUND
} yield
Ok(
Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName))
datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox
result <- (datasetBox match {
case Full(dataset) =>
Fox.successful(
Ok(
Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case Empty =>
for {
dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
result <- isAccessibleResult.header.status match {
case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case _ => Fox.failure(notFoundMessage(datasetName))
}
} yield result
case _ => Fox.failure(notFoundMessage(datasetName))
}) ?~> notFoundMessage(datasetName) ~> NOT_FOUND
} yield result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle missing datasets when accessing with GlobalAccessContext

In the case Empty branch of the getDatasetIdFromNameAndOrganization method, if datasetDAO.findOneByNameAndOrganization returns None, attempting to access dataset._id will result in a NullPointerException.

Please add error handling to manage scenarios where the dataset is not found in GlobalAccessContext to prevent potential exceptions.

Apply this diff to handle the missing dataset case:

case Empty =>
  for {
-   dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+   datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+   dataset <- datasetOption ?~> notFoundMessage(datasetName)
    isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
    result <- isAccessibleResult.header.status match {
      case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
                                             "name" -> dataset.name,
                                             "organization" -> dataset._organization,
                                             "directoryName" -> dataset.directoryName)))
      case _ => Fox.failure(notFoundMessage(datasetName))
    }
  } yield result
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox
result <- (datasetBox match {
case Full(dataset) =>
Fox.successful(
Ok(
Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case Empty =>
for {
dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
result <- isAccessibleResult.header.status match {
case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case _ => Fox.failure(notFoundMessage(datasetName))
}
} yield result
case _ => Fox.failure(notFoundMessage(datasetName))
}) ?~> notFoundMessage(datasetName) ~> NOT_FOUND
} yield result
datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox
result <- (datasetBox match {
case Full(dataset) =>
Fox.successful(
Ok(
Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case Empty =>
for {
datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
dataset <- datasetOption ?~> notFoundMessage(datasetName)
isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
result <- isAccessibleResult.header.status match {
case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case _ => Fox.failure(notFoundMessage(datasetName))
}
} yield result
case _ => Fox.failure(notFoundMessage(datasetName))
}) ?~> notFoundMessage(datasetName) ~> NOT_FOUND
} yield result

}

private def notFoundMessage(datasetName: String)(implicit ctx: DBAccessContext, m: MessagesProvider): String =
Expand Down
9 changes: 4 additions & 5 deletions app/models/dataset/Dataset.scala
Original file line number Diff line number Diff line change
Expand Up @@ -430,16 +430,15 @@ class DatasetDAO @Inject()(sqlClient: SqlClient, datasetLayerDAO: DatasetLayerDA
exists <- r.headOption
} yield exists

// Datasets are looked up by name and directoryName, as datasets from before dataset renaming was possible
// should have their directory name equal to their name during the time the link was created. This heuristic should
// have the best expected outcome as it expect to find the dataset by directoryName and it to be the oldest. In case
// someone renamed a dataset and created the link with a tool that uses the outdated dataset identification, the dataset should still be found.
// Legacy links to Datasets used their name and organizationId as identifier. In #8075 name was changed to directoryName.
// Thus, interpreting the name as the directory name should work, as changing the directory name is not possible.
// This way of looking up datasets should only be used for backwards compatibility.
def findOneByNameAndOrganization(name: String, organizationId: String)(implicit ctx: DBAccessContext): Fox[Dataset] =
for {
accessQuery <- readAccessQuery
r <- run(q"""SELECT $columns
FROM $existingCollectionName
WHERE (directoryName = $name OR name = $name)
WHERE (directoryName = $name)
AND _organization = $organizationId
AND $accessQuery
ORDER BY created ASC
Expand Down
55 changes: 25 additions & 30 deletions frontend/javascripts/components/redirect.tsx
Original file line number Diff line number Diff line change
@@ -1,46 +1,41 @@
import type { RouteComponentProps } from "react-router-dom";
import { withRouter } from "react-router-dom";
import React from "react";
import type React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the import statement for React

The import statement import type React from "react"; imports React as a type-only import, which does not include the runtime value of React. Since React is needed for JSX transformation and to use React.FC, importing it as a type-only import may cause runtime errors.

Please update the import statement to include the React namespace:

-import type React from "react";
+import React from "react";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import type React from "react";
import React from "react";

import { useEffectOnlyOnce } from "libs/react_hooks";

type Props = {
redirectTo: () => Promise<string>;
history: RouteComponentProps["history"];
pushToHistory?: boolean;
};

class AsyncRedirect extends React.PureComponent<Props> {
static defaultProps = {
pushToHistory: true,
};

componentDidMount() {
this.redirect();
}

async redirect() {
const newPath = await this.props.redirectTo();
const AsyncRedirect: React.FC<Props> = ({ redirectTo, history, pushToHistory = true }) => {
useEffectOnlyOnce(() => {
const redirect = async () => {
const newPath = await redirectTo();

if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
}
return;
}

if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (this.props.pushToHistory) {
location.assign(newPath);
if (pushToHistory) {
history.push(newPath);
} else {
location.replace(newPath);
history.replace(newPath);
}
return;
}
};

if (this.props.pushToHistory) {
this.props.history.push(newPath);
} else {
this.props.history.replace(newPath);
}
}
redirect();
});
Comment on lines +14 to +36
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling to the asynchronous redirect function

In the redirect async function, any errors thrown by redirectTo() or during the redirection process are not currently caught. This can lead to unhandled promise rejections and make debugging difficult.

Consider adding a try-catch block to handle potential errors gracefully.

Apply this diff to add error handling:

const redirect = async () => {
+  try {
    const newPath = await redirectTo();

    if (newPath.startsWith(location.origin)) {
      // The link is absolute which react-router does not support
      if (pushToHistory) {
        location.assign(newPath);
      } else {
        location.replace(newPath);
      }
      return;
    }

    if (pushToHistory) {
      history.push(newPath);
    } else {
      history.replace(newPath);
    }
+  } catch (error) {
+    console.error('Redirect failed:', error);
+    // Optionally, display an error message or navigate to an error page
+  }
};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const redirect = async () => {
const newPath = await redirectTo();
if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
}
return;
}
if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (this.props.pushToHistory) {
location.assign(newPath);
if (pushToHistory) {
history.push(newPath);
} else {
location.replace(newPath);
history.replace(newPath);
}
return;
}
};
if (this.props.pushToHistory) {
this.props.history.push(newPath);
} else {
this.props.history.replace(newPath);
}
}
redirect();
});
const redirect = async () => {
try {
const newPath = await redirectTo();
if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
}
return;
}
if (pushToHistory) {
history.push(newPath);
} else {
history.replace(newPath);
}
} catch (error) {
console.error('Redirect failed:', error);
// Optionally, display an error message or navigate to an error page
}
};
redirect();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how it has always been @philippotto. Do you think we should catch errors here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!


render() {
return null;
}
}
return null;
};

export default withRouter<RouteComponentProps & Props, any>(AsyncRedirect);