Added `IpAddress` GraphQL custom scalar for IP addresses #365

henry0715-dev · 2024-12-05T04:47:01Z

Close #353

Nic and NicInput that are not used as additional content have been
removed.

codecov · 2024-12-05T04:52:28Z

Codecov Report

Attention: Patch coverage is 89.23077% with 21 lines in your changes missing coverage. Please review.

Project coverage is 71.00%. Comparing base (09b682a) to head (4a008ea).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/graphql/ip_location.rs	0.00%	17 Missing ⚠️
src/graphql.rs	66.66%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #365      +/-   ##
==========================================
+ Coverage   70.53%   71.00%   +0.46%     
==========================================
  Files          67       67              
  Lines       14416    14545     +129     
==========================================
+ Hits        10168    10327     +159     
+ Misses       4248     4218      -30

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

sehkone · 2024-12-13T00:14:29Z

src/graphql/account.rs

@@ -651,13 +643,13 @@ impl From<types::Account> for Account {
    }
 }

-fn strings_to_ip_addrs(ip_addrs: &[String]) -> Result<Vec<IpAddr>, AddrParseError> {
+fn strings_to_ip_addrs(ip_addrs: &[IpAddress]) -> Vec<IpAddr> {


I don't think this method name makes sense anymore.

sehkone · 2024-12-13T00:15:10Z

src/graphql/account.rs

-        .map(|ip_addr| ip_addr.parse::<IpAddr>())
-        .collect::<Result<Vec<_>, _>>()?;
+        .map(|ip_addr| ip_addr.0)
+        .collect::<Vec<IpAddr>>();
    ip_addrs.sort();


sort_unstable would be a better option.

sehkone · 2024-12-13T00:18:24Z

src/graphql/account.rs

    ip_addrs.sort();
-    Ok(ip_addrs)
+    ip_addrs


I think it would be better to remove duplicate values, although it is not directly related to this issue.

I have added ip_addrs.dedup(); to remove duplicates.

sehkone · 2024-12-13T00:23:47Z

CHANGELOG.md

@@ -7,6 +7,13 @@ Versioning](https://semver.org/spec/v2.0.0.html).

 ## [Unreleased]

+### Added
+
+- Added `IpAddress` GraphQL scalar for IP addresses.


From the user's perspective, they would want to know the constraints this type enforces for validation, as well as the errors the API returns when a value doesn't meet those constraints. Therefore, I think it would be better to mention that the API returns a specific error message if the value cannot be parsed into IpAddr.

The correct term is "custom scalar", not just "scalar".

To save your time, I let you know the error message when "abc" is input:

Failed to parse "IpAddress": Invalid IP address: abc (occurred while parsing "[IpAddress!]")

henry0715-dev · 2024-12-13T01:12:26Z

@sehkone
Thank you for the PR comment.
The contents have been changed.
Thank you.

sehkone · 2024-12-15T23:26:50Z

CHANGELOG.md

+  - The API returns a specific error message if the value cannot be parsed
+    into `IpAddr`.
+    For example: `Failed to parse "IpAddress": Invalid IP address: abc(occurred
+while parsing "[IpAddress!]")`


For clarity, I suggest:

The API returns the following error message when a value cannot be parsed as an IpAddr: Failed to parse "IpAddress": Invalid IP address: abc (occurred while parsing "[IpAddress!]"), where "abc" is the input.

sehkone · 2024-12-16T07:17:47Z

CHANGELOG.md

+  - The API returns the following error message when a value cannot be parsed as
+    an `IpAddr`: `Failed to parse "IpAddress": Invalid IP address: abc (occurred
+while parsing "[IpAddress!]")`, where "abc" is the input.


When a quoted message overflows like this, we can use backticks to create a block instead.

- The API returns the following error message when a value cannot be parsed as an `IpAddr` (e.g., when "abc" is given): ``` Failed to parse "IpAddress": Invalid IP address: abc (occurred while parsing "[IpAddress!]") ```

There was no error in CI, so I thought it was an acceptable phrase.
We will apply it according to your suggestion.
Thank you.

sehkone · 2024-12-17T06:01:31Z

@henry0715-dev, this encounters Clippy warnings. Could you take care of them?

henry0715-dev · 2024-12-18T00:25:28Z

@henry0715-dev, this encounters Clippy warnings. Could you take care of them?

I have completed the fixes for the Clippy warnings.

sehkone · 2024-12-18T06:27:38Z

CHANGELOG.md

+- Added `IpAddress` GraphQL custom scalar for IP addresses.
+
+  - Applied it to the GraphQL APIs `ipLocation`, `ipLocationList`,
+    `insertAccount`, `updateAccount`, `insertSamplingPolicy`, and
+    `updateSamplingPolicy`.
+  - The API returns the following error message when a value cannot be parsed as
+    an `IpAddr` (e.g., when "abc" is given):
+
+    ```text
+    Failed to parse "IpAddress": Invalid IP address: abc (occurred while
+    parsing "[IpAddress!]")
+    ```


I think markdownlink requires an empty line before a backtick block, and the editor automatically inserts an empty line 15. However, these empty lines don't look good. Therefore, I suggest:

- Added `IpAddress` GraphQL custom scalar for IP addresses - Applied it to the GraphQL APIs `ipLocation`, `ipLocationList`, `insertAccount`, `updateAccount`, `insertSamplingPolicy`, and `updateSamplingPolicy`. - The API returns the following error message when a value cannot be parsed as an `IpAddr` (e.g., when "abc" is given):  ```text Failed to parse "IpAddress": Invalid IP address: abc (occurred while parsing "[IpAddress!]") ```

I believe this could prevent the failure caused by markdownlint.

Instead of what I told above, it would better to update the .markdownlint.yaml file as follows:

--- MD024: siblings_only: true MD031: false

Then, we can change the above part of CHANGELOG like:

- Added `IpAddress` GraphQL custom scalar for IP addresses - Applied it to the GraphQL APIs `ipLocation`, `ipLocationList`, `insertAccount`, `updateAccount`, `insertSamplingPolicy`, and `updateSamplingPolicy`. - The API returns the following error message when a value cannot be parsed as an `IpAddr` (e.g., when "abc" is given): ```text Failed to parse "IpAddress": Invalid IP address: abc (occurred while parsing "[IpAddress!]") ```

It's been changed.
Thanks for your help.

Close #353 `Nic` and `NicInput` that are not used as additional content have been removed.

sehkone · 2024-12-19T00:59:07Z

@henry0715-dev

I modified the commit title according to what has changed, so as to include the term "custom." And, I also modified the body to mention the issue number at the bottom, which is more conventional. I just wanted to let you know.

sehkone · 2024-12-19T01:00:54Z

@henry0715-dev,

I also changed the commit title to use an imperative verb instead of a past tense verb according to the rule.

henry0715-dev force-pushed the henry/scalar_ip_address branch from ca314ad to 34e9b45 Compare December 5, 2024 05:47

sehkone requested a review from msk December 12, 2024 23:22

sehkone reviewed Dec 13, 2024

View reviewed changes

henry0715-dev force-pushed the henry/scalar_ip_address branch 3 times, most recently from 6b2c23a to be8ed94 Compare December 13, 2024 01:10

msk approved these changes Dec 15, 2024

View reviewed changes

sehkone reviewed Dec 15, 2024

View reviewed changes

henry0715-dev force-pushed the henry/scalar_ip_address branch 2 times, most recently from 9e4c4b7 to 0e77ccb Compare December 16, 2024 03:42

sehkone reviewed Dec 16, 2024

View reviewed changes

henry0715-dev force-pushed the henry/scalar_ip_address branch 2 times, most recently from d8e66e5 to aa99349 Compare December 17, 2024 00:45

henry0715-dev force-pushed the henry/scalar_ip_address branch 3 times, most recently from 7d9cf9b to 19556a4 Compare December 18, 2024 00:13

sehkone reviewed Dec 18, 2024

View reviewed changes

henry0715-dev force-pushed the henry/scalar_ip_address branch from 19556a4 to 4493e0c Compare December 19, 2024 00:08

Added IpAddress GraphQL scalar for IP addresses

4a008ea

Close #353 `Nic` and `NicInput` that are not used as additional content have been removed.

henry0715-dev force-pushed the henry/scalar_ip_address branch from 4493e0c to 4a008ea Compare December 19, 2024 00:09

sehkone merged commit d4fe319 into main Dec 19, 2024
10 checks passed

sehkone deleted the henry/scalar_ip_address branch December 19, 2024 01:01

henry0715-dev changed the title ~~Added IpAddress GraphQL scalar for IP addresses~~ Added IpAddress GraphQL custom scalar for IP addresses Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `IpAddress` GraphQL custom scalar for IP addresses #365

Added `IpAddress` GraphQL custom scalar for IP addresses #365

henry0715-dev commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

sehkone Dec 13, 2024

sehkone Dec 13, 2024

sehkone Dec 13, 2024

henry0715-dev Dec 18, 2024

sehkone Dec 13, 2024

sehkone Dec 13, 2024 •

edited

Loading

sehkone Dec 13, 2024

henry0715-dev commented Dec 13, 2024

sehkone Dec 15, 2024

sehkone Dec 16, 2024

henry0715-dev Dec 16, 2024

sehkone commented Dec 17, 2024

henry0715-dev commented Dec 18, 2024

sehkone Dec 18, 2024 •

edited

Loading

sehkone Dec 18, 2024

henry0715-dev Dec 19, 2024

sehkone commented Dec 19, 2024

sehkone commented Dec 19, 2024

Added IpAddress GraphQL custom scalar for IP addresses #365

Added IpAddress GraphQL custom scalar for IP addresses #365

Conversation

henry0715-dev commented Dec 5, 2024 • edited Loading

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehkone Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henry0715-dev commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehkone commented Dec 17, 2024

henry0715-dev commented Dec 18, 2024

sehkone Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sehkone commented Dec 19, 2024

sehkone commented Dec 19, 2024

Added `IpAddress` GraphQL custom scalar for IP addresses #365

Added `IpAddress` GraphQL custom scalar for IP addresses #365

henry0715-dev commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

sehkone Dec 13, 2024 •

edited

Loading

sehkone Dec 18, 2024 •

edited

Loading