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

fix(data-model)!: more consistent query parameters #4978

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

0x009922
Copy link
Contributor

Context

There are some minor inconsistencies within query parameters (pagination, fetch size, sorting):

  • Impossible to construct these structures when using iroha_data_model as a library
  • Pagination limit and fetch size was u32, while pagination offset is u64. It doesn't make sense, since they are all technically limited to the maximum vec size in Iroha, which is usize, which is commonly represented as u64 in the data model.
  • Pagination offset is Option<NonZero<u64>>, where None value doesn't give any additional meaning and is just treated as zero, which is also a default value for u64.

Solution

  • Add missing constructors
  • Use u64/NonZero<u64> everywhere
  • Make pagination offset just u64 instead of Option<NonZero<u64>>
  • Re-arrange pagination fields - first is offset, second is limit (affects ordering in the constructor)

Review notes (optional)

  • For complex PRs, try to provide some information on how to approach the review more effectively.
  • For example, is there a natural order in which the affected files should be reviewed?

Checklist

  • I've read CONTRIBUTING.md.
  • (optional) I've written unit tests for the code changes.
  • All review comments have been resolved.

@0x009922 0x009922 added the Chore This is a small task that can be done at any point in time and is easier than others label Aug 14, 2024
@0x009922 0x009922 self-assigned this Aug 14, 2024
@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Aug 15, 2024
@mversic
Copy link
Contributor

mversic commented Aug 15, 2024

since they are all technically limited to the maximum vec size in Iroha, which is usize, which is commonly represented as u64 in the data model.

that is the hard upper limit, we can choose to make it tighter. Using a u64 for fetch size is unnecessary, especially when you consider that MAX_FETCH_SIZE = 10000

@0x009922
Copy link
Contributor Author

that is the hard upper limit, we can choose to make it tighter.

I am fine either way, just want to make it consistent.

Using a u64 for fetch size is unnecessary, especially when you consider that MAX_FETCH_SIZE = 10000

I see, maybe then make it even smaller, like u16?

@github-actions github-actions bot added the config-changes Changes in configuration and start up of the Iroha label Aug 20, 2024
Copy link

@BAStos525

@0x009922
Copy link
Contributor Author

@mversic as was discussed, let's keep u64.

mversic
mversic previously approved these changes Aug 21, 2024
@0x009922
Copy link
Contributor Author

Tests seem to work on my machine.

@nxsaken nxsaken force-pushed the fix/consistent-query-params branch from f158aba to c0085d1 Compare August 26, 2024 08:16
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
Signed-off-by: 0x009922 <43530070+0x009922@users.noreply.github.com>
@nxsaken nxsaken force-pushed the fix/consistent-query-params branch from c0085d1 to a365519 Compare August 26, 2024 08:47
@nxsaken nxsaken merged commit c3b6815 into hyperledger-iroha:main Aug 26, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries Chore This is a small task that can be done at any point in time and is easier than others config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants