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.

Sign up for GitHub

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

Jump to bottom

Nft allowances Range additions #8261

Merged
merged 27 commits into from
May 24, 2024
Merged

Nft allowances Range additions #8261

merged 27 commits into from
May 24, 2024

Conversation

mgoelswirlds
Copy link
Member

@mgoelswirlds mgoelswirlds commented May 9, 2024 •
edited by steven-sheehy

Description:
This PR adds the support for greater and lower bound conditions for query parameters in the request.

This PR modifies

  • The service layer and the repository layer to handle greater and lower bound conditions for query parameters.
  • Adds extra parameter validations as per the openapi spec
  • Shortcuts invalid ranges

Related issue(s):

Fixes #8176

Notes for reviewer:

  • This PR will have changes to the next link depending on the Next link PR
  • This PR does not add all validations to Controller tests since they are validated in services tests and these take longer to execute.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@mgoelswirlds
Adding query param checks.
9069349 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Merge remote-tracking branch 'origin/main' into 8176-Nft-allowances-e…
0e76cd5 
…nhancement
@mgoelswirlds
- Add range shortcuts to service layer
abdeb28 
- Add validations in service layer
- handle lt and gt for account.id and token.id
- Add tests.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Adding list size validation for token.id as well.
f07191f 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Fixing existing tests and adding new ones.
a3878d5 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@codecov Codecov
Copy link

codecov bot commented May 9, 2024 •
edited

Codecov Report

Attention: Patch coverage is 97.97980% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 92.46%. Comparing base (4b9bf04) to head (9b22015).
Report is 22 commits behind head on main.

Files Patch % Lines
...java/com/hedera/mirror/restjava/service/Bound.java 96.36% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #8261      +/-   ##
============================================
+ Coverage     92.30%   92.46%   +0.15%     
+ Complexity     7292     7239      -53     
============================================
  Files           899      883      -16     
  Lines         29366    29199     -167     
  Branches       3585     3580       -5     
============================================
- Hits          27107    26998     -109     
+ Misses         1439     1385      -54     
+ Partials        820      816       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgoelswirlds
- Adding a case to the repository test.
e7bd2de 
- Adding eq and range validation to the service layer
- Adding a test case for the above validation.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds mgoelswirlds self-assigned this May 9, 2024
@mgoelswirlds mgoelswirlds added enhancement Type: New feature nft Non-Fungible Token rest-java Area: Java REST API labels May 9, 2024
@mgoelswirlds mgoelswirlds added this to the 0.105.0 milestone May 9, 2024
@mgoelswirlds
Fixing a deleted condition in the repository.
1b62d90 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Fixing the optimization to have the lower bound.
54224bd 
Fixing the lower and upper bound convention.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
- Renaming params
1cd1dee 
- Adding more conditions to tests.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds mgoelswirlds requested a review from a team May 10, 2024 00:56
@mgoelswirlds mgoelswirlds marked this pull request as ready for review May 10, 2024 00:58
@mgoelswirlds
Addressing some variable rename comments.
fc1d1e3 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Renaming to remove redundancy.
c0eabf3 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Reordering and method name cleanup.
38835ab 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Throwing an error when an invalid range is provided.Fixing relevant t…
ad2158b 
…ests.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
steven-sheehy
steven-sheehy requested changes May 20, 2024
View reviewed changes
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
...rror-rest-java/src/main/java/com/hedera/mirror/restjava/service/NftAllowanceServiceImpl.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
...rror-rest-java/src/main/java/com/hedera/mirror/restjava/service/NftAllowanceServiceImpl.java Outdated Show resolved Hide resolved
...rror-rest-java/src/main/java/com/hedera/mirror/restjava/service/NftAllowanceServiceImpl.java Outdated Show resolved Hide resolved
@mgoelswirlds
Addressing PR comments.
bf4e873 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Changing a test to match the scenario.
62307f3 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
xin-hedera
xin-hedera requested changes May 21, 2024
View reviewed changes
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restjava/service/Bound.java Outdated Show resolved Hide resolved
...rror-rest-java/src/main/java/com/hedera/mirror/restjava/service/NftAllowanceServiceImpl.java Outdated Show resolved Hide resolved
...rror-rest-java/src/main/java/com/hedera/mirror/restjava/service/NftAllowanceServiceImpl.java Outdated Show resolved Hide resolved
...va/src/main/java/com/hedera/mirror/restjava/repository/NftAllowanceRepositoryCustomImpl.java Outdated Show resolved Hide resolved
@mgoelswirlds
Returning only approved_for_all=true results.
81e8fb1 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Addressing PR comments and handling corner cases.
8aac5f0 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
cleanup.
01c9ef3 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
mgoelswirlds and others added 3 commits May 23, 2024 10:17
@mgoelswirlds @steven-sheehy
Update hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restja…
d1828ca 
…va/service/Bound.java

Co-authored-by: Steven Sheehy <17552371+steven-sheehy@users.noreply.github.com>
Signed-off-by: Mugdha Goel <106084778+mgoelswirlds@users.noreply.github.com>
@mgoelswirlds @steven-sheehy
Update hedera-mirror-rest-java/src/main/java/com/hedera/mirror/restja…
b5f9e02 
…va/service/Bound.java

Co-authored-by: Steven Sheehy <17552371+steven-sheehy@users.noreply.github.com>
Signed-off-by: Mugdha Goel <106084778+mgoelswirlds@users.noreply.github.com>
@mgoelswirlds
Fixing compilation errors from PR comments.
cc99ff7 
Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@mgoelswirlds
Adding parameter name to the bound constructor for specific error mes…
9b22015 
…sages.

Addressing PR comments.

Signed-off-by: mgoelswirlds <mugdha.goel@swirldslabs.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

xin-hedera
xin-hedera approved these changes May 23, 2024
View reviewed changes
Copy link
Collaborator

@xin-hedera xin-hedera left a comment

Choose a reason for hiding this comment

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

LGTM. in case of any minor issues, can address in a follow-up PR

steven-sheehy
steven-sheehy approved these changes May 23, 2024
View reviewed changes
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM

@mgoelswirlds mgoelswirlds merged commit 81bc41f into main May 24, 2024
29 checks passed
@mgoelswirlds mgoelswirlds deleted the 8176-Nft-allowances-enhancement branch May 24, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steven-sheehy steven-sheehy steven-sheehy approved these changes

@xin-hedera xin-hedera xin-hedera approved these changes

@edwin-greene edwin-greene Awaiting requested review from edwin-greene

@jascks jascks Awaiting requested review from jascks

Assignees

@mgoelswirlds mgoelswirlds

Labels
enhancement Type: New feature nft Non-Fungible Token rest-java Area: Java REST API
Projects
None yet
Milestone
0.106.0
Development

Successfully merging this pull request may close these issues.

NFT allowance rest API enhancement and validations
5 participants
@mgoelswirlds @xin-hedera @jascks @steven-sheehy @edwin-greene