Handle composite pk filters (#18016)

* Handle composite PK filters as well as filtering by non unique PK fields

* unskip repro

* fix

* fix

* fixes

frontend/src/metabase-lib/lib/Question.js

@@ -712,6 +712,11 @@ export default class Question {
     return Mode.forQuestion(this);
   }
 
+  /**
+   * Returns true if, based on filters and table columns, the expected result is a single row.
+   * However, it might not be true when a PK column is not unique, leading to multiple rows.
+   * Because of that, always check query results in addition to this property.
+   */
   isObjectDetail(): boolean {
     const mode = this.mode();
     return mode ? mode.name() === "object" : false;

frontend/src/metabase/modes/lib/modes.js

@@ -14,6 +14,23 @@ import type Question from "metabase-lib/lib/Question";
 import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
 import NativeQuery from "metabase-lib/lib/queries/NativeQuery";
 
+const isPKFilter = (filters, query) => {
+  const sourceTablePKFields =
+    query?.table()?.fields.filter(field => field.isPK()) || [];
+
+  if (sourceTablePKFields.length === 0) {
+    return false;
+  }
+
+  const hasEqualityFilterForEveryPK = sourceTablePKFields.every(pkField => {
+    const filter = filters.find(filter => filter.field()?.id === pkField.id);
+
+    return filter?.operatorName() === "=" && filter?.arguments().length === 1;
+  });
+
+  return hasEqualityFilterForEveryPK;
+};
+
 export function getMode(question: ?Question): ?QueryMode {
   if (!question) {
     return null;
@@ -31,23 +48,7 @@ export function getMode(question: ?Question): ?QueryMode {
     const filters = query.filters();
 
     if (aggregations.length === 0 && breakouts.length === 0) {
-      const isPKFilterWithOneID = filter => {
-        if (filter.isFieldFilter()) {
-          const field = filter.field();
-          if (
-            field &&
-            field.isPK() &&
-            field.table &&
-            field.table.id === query.sourceTableId() &&
-            filter.operatorName() === "=" &&
-            filter.arguments().length === 1
-          ) {
-            return true;
-          }
-        }
-        return false;
-      };
-      if (filters.some(isPKFilterWithOneID)) {
+      if (isPKFilter(filters, query)) {
         return ObjectMode;
       } else {
         return SegmentMode;

frontend/src/metabase/query_builder/components/view/QuestionDataSource.jsx

@@ -18,10 +18,10 @@ const QuestionDataSource = ({ question, subHead, noLink, ...props }) => {
   );
 };
 
-QuestionDataSource.shouldRender = ({ question }) =>
-  getDataSourceParts({ question }).length > 0;
+QuestionDataSource.shouldRender = ({ question, isObjectDetail }) =>
+  getDataSourceParts({ question, isObjectDetail }).length > 0;
 
-function getDataSourceParts({ question, noLink, subHead }) {
+function getDataSourceParts({ question, noLink, subHead, isObjectDetail }) {
   if (!question) {
     return [];
   }
@@ -51,8 +51,6 @@ function getDataSourceParts({ question, noLink, subHead }) {
     });
   }
 
-  const isObjectDetail = question.isObjectDetail();
-
   if (table) {
     let name = table.displayName();
     if (query instanceof StructuredQuery) {

frontend/src/metabase/query_builder/components/view/QuestionDescription.jsx

@@ -7,7 +7,7 @@ import QuestionDataSource from "./QuestionDataSource";
 
 import StructuredQuery from "metabase-lib/lib/queries/StructuredQuery";
 
-const QuestionDescription = ({ question }) => {
+const QuestionDescription = ({ question, isObjectDetail }) => {
   const query = question.query();
   if (query instanceof StructuredQuery) {
     const topQuery = query.topLevelQuery();
@@ -46,7 +46,9 @@ const QuestionDescription = ({ question }) => {
     }
   }
   if (question.database()) {
-    return <QuestionDataSource question={question} />;
+    return (
+      <QuestionDataSource question={question} isObjectDetail={isObjectDetail} />
+    );
   } else {
     return <span>{t`New question`}</span>;
   }

frontend/src/metabase/query_builder/components/view/QuestionFilters.jsx

@@ -101,15 +101,23 @@ export function QuestionFilterWidget({
   );
 }
 
-QuestionFilters.shouldRender = ({ question, queryBuilderMode }) =>
+QuestionFilters.shouldRender = ({
+  question,
+  queryBuilderMode,
+  isObjectDetail,
+}) =>
   queryBuilderMode === "view" &&
   question.isStructured() &&
   question.query().isEditable() &&
   question.query().topLevelFilters().length > 0 &&
-  !question.isObjectDetail();
+  !isObjectDetail;
 
-QuestionFilterWidget.shouldRender = ({ question, queryBuilderMode }) =>
+QuestionFilterWidget.shouldRender = ({
+  question,
+  queryBuilderMode,
+  isObjectDetail,
+}) =>
   queryBuilderMode === "view" &&
   question.isStructured() &&
   question.query().isEditable() &&
-  !question.isObjectDetail();
+  !isObjectDetail;

frontend/src/metabase/query_builder/components/view/QuestionSummaries.jsx

@@ -63,7 +63,11 @@ export function QuestionSummarizeWidget({
   );
 }
 
-QuestionSummaries.shouldRender = ({ question, queryBuilderMode }) =>
+QuestionSummaries.shouldRender = ({
+  question,
+  queryBuilderMode,
+  isObjectDetail,
+}) =>
   queryBuilderMode === "view" &&
   question &&
   question.isStructured() &&
@@ -71,12 +75,16 @@ QuestionSummaries.shouldRender = ({ question, queryBuilderMode }) =>
     .query()
     .topLevelQuery()
     .hasAggregations() &&
-  !question.isObjectDetail();
+  !isObjectDetail;
 
-QuestionSummarizeWidget.shouldRender = ({ question, queryBuilderMode }) =>
+QuestionSummarizeWidget.shouldRender = ({
+  question,
+  queryBuilderMode,
+  isObjectDetail,
+}) =>
   queryBuilderMode === "view" &&
   question &&
   question.isStructured() &&
   question.query().isEditable() &&
   question.query().table() &&
-  !question.isObjectDetail();
+  !isObjectDetail;

frontend/src/metabase/query_builder/components/view/ViewHeader.jsx

@@ -44,6 +44,7 @@ const viewTitleHeaderPropTypes = {
   isShowingFilterSidebar: PropTypes.bool,
   isShowingSummarySidebar: PropTypes.bool,
   isShowingQuestionDetailsSidebar: PropTypes.bool,
+  isObjectDetail: PropTypes.bool,
 
   runQuestionQuery: PropTypes.func,
   cancelQuery: PropTypes.func,
@@ -121,6 +122,7 @@ export class ViewTitleHeader extends React.Component {
       onOpenQuestionDetails,
       onCloseQuestionDetails,
       onOpenQuestionHistory,
+      isObjectDetail,
     } = this.props;
     const { isFiltersExpanded } = this.state;
     const isShowingNotebook = queryBuilderMode === "notebook";
@@ -173,10 +175,11 @@ export class ViewTitleHeader extends React.Component {
                 collectionId={question.collectionId()}
               />
 
-              {QuestionDataSource.shouldRender({ question }) && (
+              {QuestionDataSource.shouldRender(this.props) && (
                 <QuestionDataSource
                   className="ml3 mb1"
                   question={question}
+                  isObjectDetail={isObjectDetail}
                   subHead
                 />
               )}
@@ -199,7 +202,10 @@ export class ViewTitleHeader extends React.Component {
                 {isNative ? (
                   t`New question`
                 ) : (
-                  <QuestionDescription question={question} />
+                  <QuestionDescription
+                    question={question}
+                    isObjectDetail={isObjectDetail}
+                  />
                 )}
               </ViewHeading>
               {showFiltersInHeading &&
@@ -225,6 +231,7 @@ export class ViewTitleHeader extends React.Component {
                 <QuestionDataSource
                   className="mb1"
                   question={question}
+                  isObjectDetail={isObjectDetail}
                   subHead
                   data-metabase-event={`Question Data Source Click`}
                 />

frontend/src/metabase/query_builder/selectors.js

@@ -250,8 +250,12 @@ export const getMode = createSelector(
 );
 
 export const getIsObjectDetail = createSelector(
-  [getMode],
-  mode => mode && mode.name() === "object",
+  [getMode, getQueryResults],
+  (mode, results) => {
+    // It handles filtering by a manually set PK column that is not unique
+    const hasMultipleRows = results?.some(({ data }) => data?.rows.length > 1);
+    return mode?.name() === "object" && !hasMultipleRows;
+  },
 );
 
 export const getIsDirty = createSelector(
@@ -302,6 +306,7 @@ export const getRawSeries = createSelector(
   ) => {
     let display = question && question.display();
     let settings = question && question.settings();
+
     if (isObjectDetail) {
       display = "object";
     } else if (isShowingRawTable) {

frontend/test/metabase/scenarios/question/new.cy.spec.js

@@ -315,7 +315,7 @@ describe("scenarios > question > new", () => {
       cy.url().should("include", "question#");
     });
 
-    it.skip("should correctly choose between 'Object Detail' and 'Table (metabase#13717)", () => {
+    it("should correctly choose between 'Object Detail' and 'Table (metabase#13717)", () => {
       // set ID to `No semantic type`
       cy.request("PUT", `/api/field/${ORDERS.ID}`, {
         semantic_type: null,
@@ -340,7 +340,7 @@ describe("scenarios > question > new", () => {
       cy.log(
         "**It should display the table with all orders with the selected quantity.**",
       );
-      cy.findByText("Fantastic Wool Shirt"); // order ID#3 with the same quantity
+      cy.get(".TableInteractive");
     });
 
     it("should display date granularity on Summarize when opened from saved question (metabase#11439)", () => {