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

Replace silent failures with explicit failures in index-level.ts #633

Open
Tracked by #806
thehenrytsai opened this issue Nov 29, 2023 · 7 comments
Open
Tracked by #806
Assignees
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest For the hacking month of October refactoring Code refactoring with no functional impact

Comments

@thehenrytsai
Copy link
Member

In index-level.ts, there are at least a couple of places where we accept invalid input and either return void/empty array instead of throwing explicit errors.

It is probably better to throw instead so that the caller does not assume the method succeeds with no results:

  1. queryWithInMemoryPaging()
      // if a cursor is provided but we cannot find it, we return an empty result set
      return [];
  1. getStartingKeyForCursor()
    if (indexes === undefined) {
      // invalid itemId
      return;
    }

    const sortValue = indexes[property];
    if (sortValue === undefined) {
      // invalid sort property
      return;
    }
@thehenrytsai thehenrytsai added good first issue Good for newcomers refactoring Code refactoring with no functional impact labels Nov 29, 2023
@lchauha
Copy link

lchauha commented Nov 30, 2023

Hi @thehenrytsai I would like to work on this issue. Can you please assign this to me?

@lchauha
Copy link

lchauha commented Nov 30, 2023

The two methods that you have specified above, I can't find them in index-level.ts file. Were these (queryWithInMemoryPaging(), getStartingKeyForCursor() ) examples?

@lchauha
Copy link

lchauha commented Nov 30, 2023

I have just found one place where we are not returning anything in the index-level.ts file.

https://github.com/TBD54566975/dwn-sdk-js/blob/main/src/store/index-level.ts#L157C14-L157C14, this is the only place.

Let me know if there are more of these.

@LiranCohen
Copy link
Member

Hi @lchauha! This is an Issue related to an existing PR that's being merged in. We preemptively created this Issue to address some things in a separate PR.

Keep an eye out for this PR #625 to be merged and then you will be able to pick this up.

@thehenrytsai thehenrytsai added bug Something isn't working hacktoberfest For the hacking month of October labels Sep 13, 2024
@29deepanshutyagi
Copy link

.take

Copy link

github-actions bot commented Oct 5, 2024

Thanks for taking this issue! Let us know if you have any questions!

@LiranCohen
Copy link
Member

Hey @29deepanshutyagi Have you had a chance to work on this issue? Let me know if you ran into anything that you need further explanation for.

One thing to note, it's important that both the in-memory paging and the iterator-paging should return the same errors/results for the same type of queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers hacktoberfest For the hacking month of October refactoring Code refactoring with no functional impact
Projects
None yet
Development

No branches or pull requests

4 participants