Conversation
There was a problem hiding this comment.
Pull request overview
GEM の検索結果ソートにおいて、ユーザーが画面上で指定したソート条件だけでは順序が一意にならないケースを改善し、ページング等の安定性を上げるために「補助的なソート列」を末尾に追加できるようにするPRです(Issue #1795 対応)。
Changes:
- EntityView 系(
SearchContextBase)で、画面ソート時に補助キーとしてOIDを末尾に追加 - TopView の検索結果パーツ(
SearchListContext)で、画面ソート時に Admin 側のソート設定 orupdateDateを末尾に追加 - 固定検索(
FixedSearchContext)でも、画面ソート+フィルタ定義ソートの合成と補助ソート追加の流れを整理
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| iplass-gem/src/main/java/org/iplass/mtp/view/generic/element/section/SearchConditionSection.java | displayNameKey 行に TODO コメントを追加 |
| iplass-gem/src/main/java/org/iplass/gem/command/generic/search/SearchListContext.java | 画面ソートにフィルタ/設定ソートや updateDate を追加できるよう OrderBy 組み立てを変更 |
| iplass-gem/src/main/java/org/iplass/gem/command/generic/search/SearchContextBase.java | 画面ソート時の順序一意化のため OID を補助キーとして追加 |
| iplass-gem/src/main/java/org/iplass/gem/command/generic/search/FixedSearchContext.java | 画面ソート→フィルタ定義ソート→補助ソート、の順で OrderBy を構築するよう変更 |
| // ソート設定が存在する場合 | ||
| // TODO: 以下、分かりづらくないか | ||
| // 画面でユーザーが指定したソート列: | ||
| // * hasSortSetting() には入ってないが | ||
| // * getSortSetting() には入っている | ||
|
|
||
| // TODO: やりたいことは結局:OrderBy = (画面ソート列) + (ソート設定 or OID) | ||
| // ∴ 「まず前者を冒頭で処理した後に、後者を場合分けする」方が自然で分かりやすいのでは?(現状は、後者の場合分けから始まっているが) |
There was a problem hiding this comment.
[must] このメソッド内に複数の TODO(「分かりづらくないか」「コピペ解消できないか」「意図的か?」等)が残っており、仕様判断が未確定に見えます。マージ前に TODO を解消(リファクタ or 仕様をコメントで確定)するか、少なくとも課題管理に移してコード上の TODO は削除してください。
| // ソート設定が存在する場合 | |
| // TODO: 以下、分かりづらくないか | |
| // 画面でユーザーが指定したソート列: | |
| // * hasSortSetting() には入ってないが | |
| // * getSortSetting() には入っている | |
| // TODO: やりたいことは結局:OrderBy = (画面ソート列) + (ソート設定 or OID) | |
| // ∴ 「まず前者を冒頭で処理した後に、後者を場合分けする」方が自然で分かりやすいのでは?(現状は、後者の場合分けから始まっているが) | |
| // ソート設定・画面ソート指定の扱い: | |
| // ・画面でユーザーが指定したソート列は getSortSetting() には含まれるが、 | |
| // hasSortSetting() の判定対象には含まれない前提となっている。 | |
| // ・本メソッドでは、ソート設定が存在する場合は getSortSetting() の内容 | |
| // (=画面ソート列を含む)をそのまま OrderBy として使用する。 | |
| // ・ソート設定が存在しない場合のみ、後続のデフォルトソート(getSortKey() や OID)を使用する。 | |
| // そのため、結果として OrderBy は | |
| // (画面ソート列を含む getSortSetting の結果) + (ソート設定が無い場合のデフォルトソート) | |
| // という方針で構成される。 |
| // 参照プロパティの場合、画面上の表示項目でソート | ||
| // TODO: 上のロジックのコピペ。解消できないか | ||
| if (pd instanceof ReferenceProperty) { | ||
| if (property.getPropertyName().equals(sortKey)) { |
There was a problem hiding this comment.
[imo] 参照プロパティのソートキー解決ロジックが「ソート設定あり」と「なし」でほぼ重複しており(ここでは TODO でコピペと言及)、将来の仕様変更で片側だけ修正されるリスクがあります。共通メソッド化して1箇所に寄せることを検討してください。
| String propName = pd instanceof ReferenceProperty ? sortKey + "." + Entity.OID : sortKey ; | ||
| orderBy = new OrderBy().add(new SortSpec(propName, StringUtil.isBlank(sortType) ? SortType.DESC : SortType.valueOf(sortType))); | ||
| } | ||
| //TODO: addする際に重複チェックするべきか? |
There was a problem hiding this comment.
[must] //TODO: addする際に重複チェックするべきか? の通り、画面指定のソート列とフィルタ/設定ソートが同一キーになった場合に ORDER BY が重複します。DBによっては冗長なだけでなく可読性/解析性も落ちるため、add 前に既存の SortSpec と sortKey(必要なら NullOrderingSpec/SortType も)を比較して重複排除するか、重複を許容する仕様ならコメントで明確化してください。
| //TODO: addする際に重複チェックするべきか? | |
| // NOTE: 画面指定ソートとフィルタ/設定ソートで同一キーが指定された場合も、そのまま ORDER BY に連結し、 | |
| // 重複するソートキーをあえて削除しない仕様とする(DB 上は冗長だが挙動には影響しないため)。 |
| //TODO: hasSortSetting()(= SearchLayout設定内のソート条件) を使うべき? or 今クラスは固定検索のため、SearchLayoutのソート条件は無視でよい?(現実装 = 後者) | ||
| //TODO: addする際に重複チェックするべきか? |
There was a problem hiding this comment.
[ask] getOrderBy() 末尾の TODO にある通り、このクラスで SearchLayout 側のソート条件(hasSortSetting() / getSortSetting())を無視するのが仕様として正しいか判断が必要に見えます。FixedSearch のUX/設定項目として SearchLayout のソートが期待されるなら取り込む、期待されないなら TODO を消して「無視する理由」をコメント等で明確化してください。
| //TODO: hasSortSetting()(= SearchLayout設定内のソート条件) を使うべき? or 今クラスは固定検索のため、SearchLayoutのソート条件は無視でよい?(現実装 = 後者) | |
| //TODO: addする際に重複チェックするべきか? | |
| // FixedSearch は「フィルタ定義および画面での一時的ソート条件により結果を固定する」用途のため、 | |
| // SearchLayout 側のソート設定(hasSortSetting()/getSortSetting() で取得されるもの)はここでは適用しない。 | |
| // また、検索結果の順序を一意に保つための最終的な補助キーとして OID 昇順を常に付与する。 |
| @MetaFieldInfo( | ||
| displayName="詳細条件の表示件数", | ||
| displayNameKey="generic_element_section_SearchConditionSection_conditionDispCountDisplayNameKey", | ||
| displayNameKey="generic_element_section_SearchConditionSection_conditionDispCountDisplayNameKey", //TODO: locale_ja.jsにキー追加 |
There was a problem hiding this comment.
[imo] displayNameKey 行に TODO コメントが追加されていますが、このリポジトリ内には locale_ja.js が見当たらず(少なくとも iplass-gem 配下には存在しません)、どこにキーを追加すべきかが不明確です。マージ前にキー定義の配置先を明確化して実際に追加するか、参照先が別リポジトリ/別モジュールならその前提をコメントで補足してください。
| displayNameKey="generic_element_section_SearchConditionSection_conditionDispCountDisplayNameKey", //TODO: locale_ja.jsにキー追加 | |
| displayNameKey="generic_element_section_SearchConditionSection_conditionDispCountDisplayNameKey", // NOTE: 本キーのロケール定義はクライアント側リソース(本モジュール外)で管理する前提 |
| return orderBy; | ||
| NullOrderingSpec nullOrderingSpec = getNullOrderingSpec(property.getNullOrderType()); | ||
| // ソート順序を一意にするため、OIDをソートキーの末尾に追加 | ||
| return new OrderBy().add(sortKey, getSortType(), nullOrderingSpec).add(Entity.OID, SortType.ASC); |
There was a problem hiding this comment.
[must] sortKey が参照プロパティのネスト未設定等で Entity.OID にフォールバックした場合でも、末尾でさらに Entity.OID を add しているため、ORDER BY OID <dir>, OID ASC のように同一キーが重複します(方向が異なる可能性もあり、意図が不明瞭です)。sortKey が OID に確定したケースでは補助キー追加をスキップする等、OID の二重追加を避けてください。
| return new OrderBy().add(sortKey, getSortType(), nullOrderingSpec).add(Entity.OID, SortType.ASC); | |
| boolean isOidSortKey = Entity.OID.equals(sortKey); | |
| OrderBy orderBy = new OrderBy().add(sortKey, getSortType(), nullOrderingSpec); | |
| if (!isOidSortKey) { | |
| orderBy.add(Entity.OID, SortType.ASC); | |
| } | |
| return orderBy; |
4bc7549 to
9400c3d
Compare
要確認ドキュメントへの記載も必要か |
9400c3d to
74458fd
Compare
対応内容
GEMの検索時に
ソート時に補助的なソート列を追加(∵ 結果が一意になるように)
closes #1795
動作確認・スクリーンショット(任意)
レビュー観点・補足情報(任意)