Merged
Conversation
Closed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds common Smithy shapes and pagination mixins, registers a new Store resource on the InAndOut service, and introduces Store CRUD operations, input/output shapes, geolocation and operating-hours types, and paginated ListStores. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as InAndOut Service
participant Resource as Store Resource
participant DB as Database
Client->>Service: ListStores(nextToken?, pageSize?, filters)
Service->>Resource: Resolve ListStores (apply filters, pagination)
Resource->>DB: Query stores (limit=pageSize, start=nextToken)
DB-->>Resource: Page of StoreSummaries + nextToken
Resource-->>Service: tokens + nextToken
Service-->>Client: StoreSummaries(tokens, nextToken)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
smithy/model/common.smithy (2)
86-89: Consider adding minimum constraint forpageSize.Using
NaturalNumberallowspageSize: 0, which would result in empty pages. Consider adding a minimum constraint to prevent invalid pagination requests.Proposed fix
`@httpQuery`("pageSize") `@default`(100) + `@range`(min: 1, max: 1000) `@documentation`("The maximum number of items the client is requesting to be returned in this page") - pageSize: NaturalNumber + pageSize: IntegerOr define a dedicated
PageSizetype with appropriate constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/common.smithy` around lines 86 - 89, The pageSize field uses NaturalNumber (which allows 0) and should enforce a minimum of 1 to avoid empty pages; update the pageSize declaration (symbol: pageSize) to add a minimum constraint (e.g., `@min`(1)) or replace the type with a dedicated PageSize type that enforces `@min`(1) so clients cannot request pageSize: 0 and pagination logic always receives >=1.
50-51: Naming inconsistency:PositiveDoubleallows zero.The
@range(min: 0)constraint allows zero, but mathematically "positive" excludes zero. Consider renaming toNonNegativeDoublefor accuracy, or change the constraint to@range(min: 0, exclusiveMin: true)if zero should be excluded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/common.smithy` around lines 50 - 51, The type PositiveDouble is misnamed because its `@range`(min: 0) permits zero; either rename PositiveDouble to NonNegativeDouble to reflect inclusive-zero semantics, or change the `@range` to exclude zero by adding exclusiveMin: true (i.e., update the `@range` on PositiveDouble to `@range`(min: 0, exclusiveMin: true)) so the name matches the constraint; update any references to PositiveDouble if you choose the rename.smithy/model/store/store-apis.smithy (1)
53-53: Duplicate documentation betweenGetStoreandListStores.Both operations have identical documentation: "Returns additional brand details in order to avoid multiple network round-trips". While this may be accurate for both, consider making the descriptions more specific to each operation's purpose.
Also applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/store/store-apis.smithy` at line 53, The operation docs for GetStore and ListStores are duplicated; update the `@documentation` for the GetStore and ListStores operations to be operation-specific—e.g., change GetStore's description to mention retrieving a single store by id and including brand details, and change ListStores' description to mention returning a list of stores with associated brand details to reduce round-trips; edit the `@documentation` annotations for the GetStore and ListStores operation definitions to reflect these distinct intents.smithy/model/main.smithy (1)
10-10: Consider renaming paginationitemsfield from "tokens" to "items" or "stores".The pagination configuration uses
items: "tokens", but "tokens" is an unconventional name for paginated store results. This could cause confusion since "token" typically refers to pagination cursors, not the actual data items. Consider using a more descriptive name likeitemsorstoresin both the service-level@paginatedtrait and the correspondingStoreSummaries.tokensfield.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/main.smithy` at line 10, The `@paginated` trait currently maps the paginated collection to the name "tokens", which is misleading; update the trait value and the corresponding model field to a clearer name (e.g., change `@paginated`(..., items: "tokens") to items: "items" or items: "stores") and rename the StoreSummaries.tokens field to match (e.g., StoreSummaries.items or StoreSummaries.stores), ensuring all references and generated shapes that expect the paginated items are updated accordingly.smithy/model/store/store-io.smithy (2)
29-31: Latitude and longitude should be validated together.A store location requires both
longitudeandlatitudeto be meaningful. Currently, a client could provide only one coordinate. Consider:
- Server-side validation to require both or neither
- Or grouping them into a
Locationstructure with both fields requiredThis same consideration applies to
UpdateStoreInput(lines 77-79).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/store/store-io.smithy` around lines 29 - 31, Store model currently defines longitude and latitude independently (fields "longitude" and "latitude"), allowing clients to submit only one coordinate; update the model to require both coordinates together by either (A) introducing a new "Location" structure containing required "longitude" and "latitude" and replace the two fields in the Store model and UpdateStoreInput with a single "location: Location" field, or (B) add server-side validation in the Store/UpdateStoreInput handling to enforce the rule "both or neither" (reject requests with only one of "longitude" or "latitude"); update the relevant model definitions and any serializers/parsers for Store and UpdateStoreInput to reflect the chosen approach and ensure tests cover both missing/both-coordinate cases.
54-57: Document the location-distance dependency in the operation documentation.The comment indicates
userLongitudeanduserLatitudemust be provided formaxDistancequeries to work. This constraint should be:
- Documented at the operation level (
ListStoresin store-apis.smithy) so it appears in generated API documentation- Consider server-side validation to return
InvalidInputErrorwhenmaxDistanceis provided without location🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/store/store-io.smithy` around lines 54 - 57, Update the API docs and add validation: add an operation-level documentation note on ListStores (operation ListStores in store-apis.smithy) that clearly states maxDistance requires userLongitude and userLatitude to be provided for distance-based queries, and ensure server-side input validation in the ListStores handler checks that when maxDistance is present the parameters userLongitude and userLatitude are also present — if not, return InvalidInputError; reference the request fields maxDistance, userLongitude, userLatitude and the error type InvalidInputError when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smithy/model/store/store-types.smithy`:
- Around line 21-32: The StoreSummary structure lacks the resource identifier;
add a storeId member to the StoreSummary structure (e.g., storeId: UUID) so
responses for GetStore, CreateStore, UpdateStore, and DeleteStore include the
resource ID; update the StoreSummary definition in store-types.smithy (structure
StoreSummary) and ensure any shapes/operations that return StoreSummary
(GetStore, CreateStore, UpdateStore, DeleteStore) populate storeId accordingly
and adjust any tests/docs or serializers that construct StoreSummary to include
this new field.
---
Nitpick comments:
In `@smithy/model/common.smithy`:
- Around line 86-89: The pageSize field uses NaturalNumber (which allows 0) and
should enforce a minimum of 1 to avoid empty pages; update the pageSize
declaration (symbol: pageSize) to add a minimum constraint (e.g., `@min`(1)) or
replace the type with a dedicated PageSize type that enforces `@min`(1) so clients
cannot request pageSize: 0 and pagination logic always receives >=1.
- Around line 50-51: The type PositiveDouble is misnamed because its `@range`(min:
0) permits zero; either rename PositiveDouble to NonNegativeDouble to reflect
inclusive-zero semantics, or change the `@range` to exclude zero by adding
exclusiveMin: true (i.e., update the `@range` on PositiveDouble to `@range`(min: 0,
exclusiveMin: true)) so the name matches the constraint; update any references
to PositiveDouble if you choose the rename.
In `@smithy/model/main.smithy`:
- Line 10: The `@paginated` trait currently maps the paginated collection to the
name "tokens", which is misleading; update the trait value and the corresponding
model field to a clearer name (e.g., change `@paginated`(..., items: "tokens") to
items: "items" or items: "stores") and rename the StoreSummaries.tokens field to
match (e.g., StoreSummaries.items or StoreSummaries.stores), ensuring all
references and generated shapes that expect the paginated items are updated
accordingly.
In `@smithy/model/store/store-apis.smithy`:
- Line 53: The operation docs for GetStore and ListStores are duplicated; update
the `@documentation` for the GetStore and ListStores operations to be
operation-specific—e.g., change GetStore's description to mention retrieving a
single store by id and including brand details, and change ListStores'
description to mention returning a list of stores with associated brand details
to reduce round-trips; edit the `@documentation` annotations for the GetStore and
ListStores operation definitions to reflect these distinct intents.
In `@smithy/model/store/store-io.smithy`:
- Around line 29-31: Store model currently defines longitude and latitude
independently (fields "longitude" and "latitude"), allowing clients to submit
only one coordinate; update the model to require both coordinates together by
either (A) introducing a new "Location" structure containing required
"longitude" and "latitude" and replace the two fields in the Store model and
UpdateStoreInput with a single "location: Location" field, or (B) add
server-side validation in the Store/UpdateStoreInput handling to enforce the
rule "both or neither" (reject requests with only one of "longitude" or
"latitude"); update the relevant model definitions and any serializers/parsers
for Store and UpdateStoreInput to reflect the chosen approach and ensure tests
cover both missing/both-coordinate cases.
- Around line 54-57: Update the API docs and add validation: add an
operation-level documentation note on ListStores (operation ListStores in
store-apis.smithy) that clearly states maxDistance requires userLongitude and
userLatitude to be provided for distance-based queries, and ensure server-side
input validation in the ListStores handler checks that when maxDistance is
present the parameters userLongitude and userLatitude are also present — if not,
return InvalidInputError; reference the request fields maxDistance,
userLongitude, userLatitude and the error type InvalidInputError when
implementing this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e991d2-91bc-48c9-a2f1-b842b4093b86
📒 Files selected for processing (5)
smithy/model/common.smithysmithy/model/main.smithysmithy/model/store/store-apis.smithysmithy/model/store/store-io.smithysmithy/model/store/store-types.smithy
petrubraha
commented
Mar 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smithy/model/store/store-types.smithy`:
- Around line 22-24: The `@resourceIdentifier` annotation is using the resource
shape name "Store" instead of the identifier key; update the annotation on the
storeId member to reference the identifier key name by changing
`@resourceIdentifier`("Store") to `@resourceIdentifier`("storeId") so the Store
resource's identifier is correctly bound to the storeId member.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbbcc437-ea29-4728-8af5-bde3e1623787
📒 Files selected for processing (1)
smithy/model/store/store-types.smithy
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smithy/model/store/store-types.smithy`:
- Around line 54-56: The StoreSummaries structure defines a member named tokens
but it actually contains StoreSummaryList entries; rename the member from tokens
to a clearer name (e.g., stores or items) in the StoreSummaries structure and
update all references (StoreSummaries.tokens → StoreSummaries.stores) including
the type declaration (StoreSummaryList) and any usages in operations like
ListStores, serializers/deserializers, and tests so generated response shapes
reflect the correct collection name; ensure the OutputPagination mixin usage
remains valid after the rename and update any generated model references or docs
accordingly.
- Around line 39-41: The model currently exposes longitude and latitude as two
independent optional fields (longitude, latitude) which permits half-populated
coordinates; change the shape to a single optional structure (e.g.,
GeoCoordinates) and replace the two fields with one optional field (e.g.,
geoCoordinates: GeoCoordinates) so coordinates are always present together; add
or reference a GeoCoordinates structure with required longitude and latitude
members and update any serializers/deserializers or references that used
longitude/latitude directly to read/write geoCoordinates instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a2b0e08-37f6-4655-bc04-817cbc9a7e27
📒 Files selected for processing (1)
smithy/model/store/store-types.smithy
lefcos
approved these changes
Mar 7, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
smithy/model/store/store-types.smithy (1)
50-53: Consider markingtokensas@required.For paginated responses, clients typically expect the list field to always be present (even if empty) rather than potentially null. This avoids null-checking boilerplate and provides consistent response shape.
Suggested change
structure StoreSummaries with [OutputPagination] { + `@required` tokens: StoreSummaryList }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/store/store-types.smithy` around lines 50 - 53, The StoreSummaries structure's tokens field should be marked as required so paginated responses always include an (possibly empty) list; update the StoreSummaries definition to annotate tokens with `@required` (i.e., add the required trait to the tokens member in the StoreSummaries structure) so clients receive a non-null StoreSummaryList from the StoreSummaries output.smithy/model/common.smithy (1)
50-51: Minor:PositiveDoubleallows zero.The
@range(min: 0)constraint includes zero, which is technically "non-negative" rather than "positive." If this is intentional (e.g., for quantities where zero is valid), consider renaming toNonNegativeDoublefor clarity. If zero should be excluded, use@range(min: 0, minExclusive: true)if supported, or document the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/common.smithy` around lines 50 - 51, The PositiveDouble type is currently constrained with `@range`(min: 0), which permits zero (non-negative) instead of strictly positive; either change the constraint to exclude zero (e.g., use `@range`(min: 0, minExclusive: true) or equivalent) on the PositiveDouble definition to enforce strictly >0, or if zero is allowed rename the type to NonNegativeDouble and update references and documentation accordingly—locate the PositiveDouble typedef in the Smithy model and apply the chosen fix consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@smithy/model/common.smithy`:
- Around line 50-51: The PositiveDouble type is currently constrained with
`@range`(min: 0), which permits zero (non-negative) instead of strictly positive;
either change the constraint to exclude zero (e.g., use `@range`(min: 0,
minExclusive: true) or equivalent) on the PositiveDouble definition to enforce
strictly >0, or if zero is allowed rename the type to NonNegativeDouble and
update references and documentation accordingly—locate the PositiveDouble
typedef in the Smithy model and apply the chosen fix consistently.
In `@smithy/model/store/store-types.smithy`:
- Around line 50-53: The StoreSummaries structure's tokens field should be
marked as required so paginated responses always include an (possibly empty)
list; update the StoreSummaries definition to annotate tokens with `@required`
(i.e., add the required trait to the tokens member in the StoreSummaries
structure) so clients receive a non-null StoreSummaryList from the
StoreSummaries output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e25b061c-47c4-4a0e-85a4-3f0bf0de21da
📒 Files selected for processing (4)
smithy/model/common.smithysmithy/model/store/store-apis.smithysmithy/model/store/store-io.smithysmithy/model/store/store-types.smithy
🚧 Files skipped from review as they are similar to previous changes (2)
- smithy/model/store/store-apis.smithy
- smithy/model/store/store-io.smithy
petrubraha
force-pushed
the
5-store-resource-apis
branch
from
1567b56 to
e395320
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
mappingVersionfield will be moved to floor related APIs.Summary by CodeRabbit