feat(import): support multi-vendor place imports#91
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces bearer token scoping via an ChangesToken-Scoped Import Origins and Vendor Access Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dadofsambonzuki
changed the title
|
I think we can totally remove all the category -> icon mapping. That was intended when we were going to temporarily display imported places, which we don't do. |
|
Reviewed + ran tests on the pinned 1.93.0 toolchain. One blocker.
Two worth a line in the PR:
Vendor centralization, the Happy to write the regression test or draft the fix - ping @rockstardev to point me at it. |
…coped token bypass
dadofsambonzuki
force-pushed
the
import-enhancements
branch
from
a7bbd08 to
f3eb08c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/rpc/import/revoke_submitted_place.rs (1)
24-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid panics when
idis absent and params are incomplete.Line 29 and Line 30 unwrap optional request fields. If either field is missing, this panics instead of returning a normal RPC error.
Proposed fix
let submission = match params.id { Some(id) => Some(db::main::place_submission::queries::select_by_id(id, pool).await?), None => { db::main::place_submission::queries::select_by_origin_and_external_id( - params.origin.unwrap(), - params.external_id.unwrap(), + params.origin.ok_or("missing field: origin")?, + params.external_id.ok_or("missing field: external_id")?, pool, ) .await? } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/import/revoke_submitted_place.rs` around lines 24 - 33, In run (src/rpc/import/revoke_submitted_place.rs) avoid unwrapping params.origin and params.external_id when params.id is None: check that both params.origin and params.external_id are Some and return a proper RPC error (Result::Err) when either is missing, instead of calling unwrap; then pass the unwrapped values to db::main::place_submission::queries::select_by_origin_and_external_id only after validation so the function never panics on incomplete input.src/rpc/import/get_submitted_place.rs (1)
30-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle missing lookup params and not-found rows without
unwrap.Line 34, Line 35, and Line 39 currently unwrap user-controlled/optional values. Return structured errors instead of panicking when
origin/external_idis missing or lookup returns no row.Proposed fix
pub async fn run(params: Params, roles: &[Role], token: &AccessToken, pool: &Pool) -> Result<Res> { let submission = match params.id { Some(id) => db::main::place_submission::queries::select_by_id(id, pool).await?, None => db::main::place_submission::queries::select_by_origin_and_external_id( - params.origin.unwrap(), - params.external_id.unwrap(), + params.origin.ok_or("missing field: origin")?, + params.external_id.ok_or("missing field: external_id")?, pool, ) .await? - .unwrap(), + .ok_or("can't find place with provided origin and external_id")?, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rpc/import/get_submitted_place.rs` around lines 30 - 39, The run function is unwrapping user-controlled Options and query results (Params.id, params.origin, params.external_id, and the Option from select_by_origin_and_external_id), which can panic; instead validate presence of id OR both origin and external_id up-front and return a proper error (e.g., InvalidInput/MissingParameter) if missing, then call select_by_id(id, pool) or select_by_origin_and_external_id(origin, external_id, pool) and handle the Result<Option<...>> by mapping a None to a structured NotFound error rather than calling unwrap; update the branches in run to return these structured errors when params are absent or when the DB lookup yields None so the function never panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/main/migrations/101.sql`:
- Around line 4-9: The WHERE clauses using "roles LIKE '%places_source%'" (both
the top-level filter and the subquery on the user table) should be replaced with
exact JSON membership checks: for a JSONB array use an EXISTS with
jsonb_array_elements_text(roles) = 'places_source', or if roles is an object use
json_each/json_each_text equality checks; update both the top-level predicate
and the subquery (the roles column, the user table and user_id references) to
use the appropriate json/jsonb exact-equality expression rather than LIKE so
only exact "places_source" entries match.
In `@src/rest/v4/places.rs`:
- Around line 407-411: The branch that calls
db::main::place_submission::vendor::origin_for_payment_tag(&tag_name,
&tag_value) only retains matching pending items when Some(origin) is returned
but silently skips filtering when it returns None; update the code in that block
to explicitly clear pending_matches (e.g., pending_matches.clear() or set it to
an empty collection) when origin_for_payment_tag returns None so tag filtering
does not leave prior matches intact. Ensure you modify the branch around
pending_matches.retain to handle both Some(origin) and None cases using the
tag_name/tag_value lookup result.
---
Outside diff comments:
In `@src/rpc/import/get_submitted_place.rs`:
- Around line 30-39: The run function is unwrapping user-controlled Options and
query results (Params.id, params.origin, params.external_id, and the Option from
select_by_origin_and_external_id), which can panic; instead validate presence of
id OR both origin and external_id up-front and return a proper error (e.g.,
InvalidInput/MissingParameter) if missing, then call select_by_id(id, pool) or
select_by_origin_and_external_id(origin, external_id, pool) and handle the
Result<Option<...>> by mapping a None to a structured NotFound error rather than
calling unwrap; update the branches in run to return these structured errors
when params are absent or when the DB lookup yields None so the function never
panics.
In `@src/rpc/import/revoke_submitted_place.rs`:
- Around line 24-33: In run (src/rpc/import/revoke_submitted_place.rs) avoid
unwrapping params.origin and params.external_id when params.id is None: check
that both params.origin and params.external_id are Some and return a proper RPC
error (Result::Err) when either is missing, instead of calling unwrap; then pass
the unwrapped values to
db::main::place_submission::queries::select_by_origin_and_external_id only after
validation so the function never panics on incomplete input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cf3d218-4166-4ccb-90a6-96fbf6bece72
📒 Files selected for processing (16)
docs/rpc/import/README.mdsrc/db/main/access_token/blocking_queries.rssrc/db/main/access_token/queries.rssrc/db/main/access_token/schema.rssrc/db/main/migrations/101.sqlsrc/db/main/place_submission/mod.rssrc/db/main/place_submission/schema.rssrc/db/main/place_submission/vendor.rssrc/db/main/schema.sqlsrc/rest/v4/places.rssrc/rpc/handler.rssrc/rpc/import/get_submitted_place.rssrc/rpc/import/mod.rssrc/rpc/import/revoke_submitted_place.rssrc/rpc/import/submit_place.rssrc/rpc/import/sync_submitted_places.rs
|
Verified the fix on Test suite at 325 passed / 0 failed / 3 ignored on the pinned 1.93.0 toolchain. The wildcard migration policy + silent icon collapse in |
2 participants
Need
Currently, the
/importendpoint only supports Square as an origin. Multiple other wallets have expressed interest in using this endpoint to leverage the verification and quality controls provided by taggers and avoid the pitfalls associated with direct OSM integration.Summary
*wildcard support for all origins.Validation
cargo fmtcargo clippy -- -D warningscargo testOut Of Scope
Summary by CodeRabbit
New Features
Improvements
Documentation