Skip to content

fix(import): return descriptive error messages and success response #165

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

Open
behdadmansouri wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(import): return descriptive error messages and success response #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bce6e27
fix(import): return descriptive error messages and success response
behdadmansouri Jun 17, 2026
9210d8c
fix(import): make multi-bucket import atomic with rollback on failure
behdadmansouri Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions aw_server/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,11 @@ def import_bucket(self, bucket_data: Any):
bucket_id = bucket_data["id"]
logger.info(f"Importing bucket {bucket_id}")

# TODO: Check that bucket doesn't already exist
if bucket_id in self.db.buckets():
raise Exception(
f"Bucket '{bucket_id}' already exists. Delete it first or rename the bucket before importing."
)
Comment on lines +110 to +113

@greptile-apps greptile-apps Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Raising a bare Exception means the caller cannot distinguish this user-facing validation error from unexpected runtime errors. A more specific type (e.g., ValueError) lets rest.py selectively catch expected errors and log them at WARNING level rather than ERROR, while letting unexpected exceptions propagate differently if needed in the future.

Suggested change
if bucket_id in self.db.buckets():
raise Exception(
f"Bucket '{bucket_id}' already exists. Delete it first or rename the bucket before importing."
)
if bucket_id in self.db.buckets():
raise ValueError(
f"Bucket '{bucket_id}' already exists. Delete it first or rename the bucket before importing."
)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


self.db.create_bucket(
bucket_id,
type=bucket_data["type"],
Expand All @@ -132,8 +136,15 @@ def import_bucket(self, bucket_data: Any):
)

def import_all(self, buckets: Dict[str, Any]):
for bid, bucket in buckets.items():
self.import_bucket(bucket)
imported: List[str] = []
try:
for _bid, bucket in buckets.items():
self.import_bucket(bucket)
imported.append(bucket["id"])
except Exception:
for bid in imported:
self.db.delete_bucket(bid)
raise

def create_bucket(
self,
Expand Down
26 changes: 15 additions & 11 deletions aw_server/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,18 +370,22 @@ class ImportAllResource(Resource):
@api.expect(buckets_export)
@copy_doc(ServerAPI.import_all)
def post(self):
# If import comes from a form in th web-ui
if len(request.files) > 0:
# web-ui form only allows one file, but technically it's possible to
# upload multiple files at the same time
for filename, f in request.files.items():
buckets = json.loads(f.stream.read())["buckets"]
try:
# If import comes from a form in the web-ui
if len(request.files) > 0:
# web-ui form only allows one file, but technically it's possible to
# upload multiple files at the same time
for filename, f in request.files.items():
buckets = json.loads(f.stream.read())["buckets"]
current_app.api.import_all(buckets)
# Normal import from body
else:
buckets = request.get_json()["buckets"]
current_app.api.import_all(buckets)
# Normal import from body
else:
buckets = request.get_json()["buckets"]
current_app.api.import_all(buckets)
return None, 200
except Exception as e:
logger.exception("Import failed")
return {"message": str(e)}, 400
Comment on lines +385 to +387

@greptile-apps greptile-apps Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 logger.exception emits a full traceback. For the expected "bucket already exists" case this is noise — it's a user-facing validation error, not an unexpected server error. Logging it at WARNING (without the traceback) avoids polluting the log with stack traces for routine user mistakes, while keeping real surprises at ERROR/EXCEPTION level.

Suggested change
except Exception as e:
logger.exception("Import failed")
return {"message": str(e)}, 400
except Exception as e:
logger.warning("Import failed: %s", e)
return {"message": str(e)}, 400

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

return {"message": "Import successful"}, 200


# LOGGING
Expand Down