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

feat!: Add traits for iterable and non-iterable queries #4638

Merged
merged 5 commits into from
May 28, 2024

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented May 22, 2024

Description

This PR is a first step towards more type-safe queries (#4569). It adds traits for singular and iterable queries and prohibits supplying filters, sorting, pagination and batching parameters in compile time (it is already a runtime error to do so).

Linked issue

Partially addresses #4569

Benefits

  • less error prone API
  • lays foundation to allow type-checking query filters

Checklist

  • make CI pass

@DCNick3 DCNick3 changed the title Add traits for iterable and non-iterable queries feat!: Add traits for iterable and non-iterable queries May 22, 2024
0x009922
0x009922 previously approved these changes May 23, 2024
Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

LGTM.

I noticed that smart_contract and client duplicate impls of query execution, and both have type logic to guard against incorrect use of queries. But this compile-time guard is tested only for client. Would it make sense to make a ui test for smartcontracts as well?

Also, do I understand correctly that the runtime check you are talking about is this code?

// nothing applies to the singular results
LazyQueryOutput::QueryOutput(output) => {
if filter != &PredicateBox::default()
|| sorting != &Sorting::default()
|| pagination != Pagination::default()
|| fetch_size != FetchSize::default()
{
return Err(Error::InvalidSingularParameters);
}
Ok(ProcessedQueryOutput::Single(output))
}

@0x009922 0x009922 self-assigned this May 23, 2024
@DCNick3
Copy link
Contributor Author

DCNick3 commented May 23, 2024

I noticed that smart_contract and client duplicate impls of query execution, and both have type logic to guard against incorrect use of queries. But this compile-time guard is tested only for client. Would it make sense to make a ui test for smartcontracts as well?

I thought about that too, though I just got lazy doing that 😅. I guess I'll update the PR to add that

Also, do I understand correctly that the runtime check you are talking about is this code?

Yes, this is the check I added in #4544 as per your suggestion =)

client/tests/ui_fail/cant_filter_singular_query.stderr Outdated Show resolved Hide resolved
smart_contract/Cargo.toml Outdated Show resolved Hide resolved
@Erigara
Copy link
Contributor

Erigara commented May 23, 2024

Other than comments LGTM

Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
…queries

This only changes the interfaces (client & smart contract) to be less error-prone, doing so is already a runtime error

Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
…racts

Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
…racts

Signed-off-by: Nikita Strygin <dcnick3@users.noreply.github.com>
@DCNick3 DCNick3 merged commit e0ab50c into hyperledger:main May 28, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants