クエリパラメータで size / next / prev のいずれかを指定した場合 Chunkable インスタンスの direction を @ChunkableDefault の値に設定する
#51
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
…n を `@ChunkableDefault` の値に設定する
katagiri-kazumune
force-pushed
the
feature/fix-issue-50
branch
from
897cb93 to
fbb47d9
Compare
| pageSize = pageSize > maxPageSize ? maxPageSize : pageSize; | ||
|
|
||
| Direction direction = Direction.fromOptionalString(directionString).orElse(null); | ||
| Direction direction = Direction.fromOptionalString(directionString).orElse(defaultOrFallback.getDirection()); |
Collaborator
Author
There was a problem hiding this comment.
directionString はクエリパラメータ direction の値。
defaultOrFallback は、@ChunkableDefault の設定値を初期値にした Chunkable インスタンス。
以前は クエリパラメータ direction が null もしくは Direction に含まない文字列を指定した時、null を返していた。
| assertThat(actualChunkable.getPaginationRelation(), is(PaginationRelation.PREV)); | ||
| assertThat(actualChunkable.getPaginationToken(), is("prev_token")); | ||
| assertThat(actualChunkable.getMaxPageSize(), is(12)); | ||
| assertThat(actualChunkable.getDirection(), is(Direction.DESC)); |
Collaborator
Author
There was a problem hiding this comment.
DESC を指定したら DESC
| assertThat(actualChunkable.getPaginationRelation(), is(nullValue())); | ||
| assertThat(actualChunkable.getPaginationToken(), is(nullValue())); | ||
| assertThat(actualChunkable.getMaxPageSize(), is(12)); | ||
| assertThat(actualChunkable.getDirection(), is(Direction.ASC)); |
Collaborator
Author
There was a problem hiding this comment.
ASC を指定したら ASC
| assertThat(actualChunkable.getPaginationRelation(), is(nullValue())); | ||
| assertThat(actualChunkable.getPaginationToken(), is(nullValue())); | ||
| assertThat(actualChunkable.getMaxPageSize(), is(12)); | ||
| assertThat(actualChunkable.getDirection(), is(Direction.DESC)); |
Collaborator
Author
There was a problem hiding this comment.
クエリパラメータ未指定の時は @ChunkableDefault の初期値を使用する
(この動きは変えていない)
| assertThat(actualChunkable.getPaginationToken(), is("next_token")); | ||
| assertThat(actualChunkable.getMaxPageSize(), is(1234)); | ||
| // defaultHandlerWithInitValue に付与した ChunkableDefault の direction の初期値は DESC | ||
| assertThat(actualChunkable.getDirection(), is(Direction.DESC)); |
Collaborator
Author
There was a problem hiding this comment.
size や next 指定時でも、direction 未指定時は ChunkableDefault の初期値を使用する
| assertThat(actualChunkable.getPaginationToken(), is(nullValue())); | ||
| assertThat(actualChunkable.getMaxPageSize(), is(123)); | ||
| assertThat(actualChunkable.getDirection(), is(nullValue())); | ||
| assertThat(actualChunkable.getDirection(), is(Direction.ASC)); // ChunkableDefault の direction の初期値は ASC |
Collaborator
Author
There was a problem hiding this comment.
null を返していたが、@ChunkableDefault の初期値 ASC を返すようにした
katagiri-kazumune
requested review from
cm-fujimura,
inabajunmr,
komuro-hiraku and
tututen
inabajunmr
approved these changes
Oct 19, 2020
| ModelAndViewContainer mavContainer = mock(ModelAndViewContainer.class); | ||
| NativeWebRequest webRequest = mock(NativeWebRequest.class); | ||
| // size と next 指定 | ||
| when(webRequest.getParameter(eq("direction"))).thenReturn(Direction.DESC.name()); |
Contributor
There was a problem hiding this comment.
デフォルトDESCなのでここはASCのがテスト的によいきがしますがどうでしょ
Collaborator
Author
There was a problem hiding this comment.
そういえば direction のデフォルトとリクエストが一緒だった時のテストがなかったなぁと思ったのであえての DESC にしたのでした。特に問題がなければこのままで。
(コメントは間違ってたので修正しました)
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.
内容
See #50
@ChunkableDefault(size = 10, direction = Sort.Direction.DESC)を設定している時、なのに
なのはおかしいので修正。
spring-data-mirage の場合、direction が null の場合 ASC 扱いとしているので問題にはなっていないが、direction の初期値の扱いが明らかにおかしい。
(direction が未指定の場合に使用する値となるべき)
この対応で発生しうる不具合
ケース1
@ChunkableDefault(size = 10, direction = Sort.Direction.DESC)を指定した時にクエリパラメータで size / next / prev のいずれかを指定していると Chunkable インスタンスの direction は null
を期待している処理が存在する場合
↓
そんな期待するのは矛盾していると思われるし、現在の Pz で
@ChunkableDefaultの direction を指定しているのは1つだけだったので問題無しと判断。(braking-change ではない筈)
ケース2
@ChunkableDefaultを指定した時(初期値の変更無し)にクエリパラメータで size の指定をして、Chunkable インスタンスの direction に null
を期待している処理が存在する場合(対応後は ASC が設定されてしまうのでエラーになる)
↓
spring-data-mirage の場合、direction が null の場合 ASC 扱いとしているので問題ない筈。
クエリパラメータに ASC 指定された時に direction には ASC が指定されるので ASC での動作確認は行っているハズ。
むしろ既存の振る舞いを受け入れているのに ASC の対応をしていない方がおかしい。