dawarich icon indicating copy to clipboard operation
dawarich copied to clipboard

/overland/batches needs better error handling

Open sunstep opened this issue 10 months ago • 1 comments

Version 0.23.5

Describe the bug Whenever I try to use the /overland/batches endpoint to upload my batch of points, it will always give me an OK in the response body and a 201 status code indicating that everything went successfully, but my points are not appearing in the API. I already checked my queues, and all are empty.

It seems like something is internally going wrong at Dawarich, which could be due to bad data that I sent to the API endpoint, but the problem is that Dawarich seems to have no error handling for this endpoint.

Any reason for the endpoint to NOT make a new point should be reported to the client that requested the API end point. So the developer can solve the problem using the reason.

To Reproduce Steps to reproduce the behavior:

  1. Go to a tool like reqbin.com
  2. Enter the following URL: http(s)://YOUR_HOST/api/v1/overland/batches?api_key=YOUR_API_KEY
  3. Use the following request body:

{ "locations" : [ { "type" : "Feature", "geometry" : { "type" : "Point", "coordinates" : [ 0.0, 0.0] }, "properties" : { "timestamp" : "2025-02-01T16:40:40Z", "altitude" : 50.70000076293945, "speed" : 0.0, "horizontal_accuracy" : 11.477999687194824, "vertical_accuracy" : 1.0, "motion" : [], "pauses" : false, "activity" : "", "desired_accuracy" : 0.0, "deferred" : 0.0, "significant_change" : "", "locations_in_payload" : 0, "device_id" : "SM-S916B", "wifi" : "Censored", "battery_state" : "connectedNotCharging", "battery_level" : 79 } } ] }

Expected behavior As the response indicates, a new point should be created. But what actually happens, is that Dawarich acknowledges that a point has been received but it seems to go wrong somewhere while processing it. It should give a message indicating what goes wrong, instead of giving a 201 status code with an OK message.

Logs If applicable, add logs from containers dawarich_app and dawarich_sidekiq to help explain your problem.

I did not find much but here is what I have:

I, [2025-02-01T17:22:25.036987 #123] INFO -- : ↳ app/controllers/api/v1/overland/batches_controller.rb:5:in create' I, [2025-02-01T17:22:25.037429 #123] INFO -- : {"method":"POST","path":"/api/v1/overland/batches","format":"json","controller":"Api::V1::Overland::BatchesController","action":"create","status":201,"allocations":1872,"duration":7.55,"view":0.19,"db":0.74,"unpermitted_params":["api_key"]} D, [2025-02-01T17:22:26.123937 #123] DEBUG -- : User Load (0.3ms) SELECT "users".* FROM "users" WHERE "users"."api_key" = $1 LIMIT $2 [["api_key", "[FILTERED]"], ["LIMIT", 1]] D, [2025-02-01T17:22:26.124303 #123] DEBUG -- : ↳ app/controllers/api_controller.rb:16:in current_api_user' I, [2025-02-01T17:22:26.125311 #123] INFO -- : Enqueued Overland::BatchCreatingJob (Job ID: 8f8efe41-ad6c-4e38-9879-1d77cf95f9d6) to Sidekiq(default) with arguments: #<ActionController::Parameters {"locations"=>[#<ActionController::Parameters {"type"=>"Feature", "geometry"=>#<ActionController::Parameters {"type"=>"Point", "coordinates"=>[0.0, 0.0]} permitted: true>, "properties"=>#<ActionController::Parameters {"timestamp"=>"2025-02-01T16:40:40Z", "altitude"=>50.70000076293945, "speed"=>0.0, "horizontal_accuracy"=>11.477999687194824, "vertical_accuracy"=>1.0, "motion"=>[], "pauses"=>false, "activity"=>"", "desired_accuracy"=>0.0, "deferred"=>0.0, "significant_change"=>"", "locations_in_payload"=>0, "device_id"=>"SM-S916B", "wifi"=>"Censored", "battery_state"=>"connectedNotCharging", "battery_level"=>79} permitted: true>} permitted: true>], "batch"=>#<ActionController::Parameters {"locations"=>[#<ActionController::Parameters {"type"=>"Feature", "geometry"=>#<ActionController::Parameters {"type"=>"Point", "coordinates"=>[0.0, 0.0]} permitted: true>, "properties"=>#<ActionController::Parameters {"timestamp"=>"2025-02-01T16:40:40Z", "altitude"=>50.70000076293945, "speed"=>0.0, "horizontal_accuracy"=>11.477999687194824, "vertical_accuracy"=>1.0, "motion"=>[], "pauses"=>false, "activity"=>"", "desired_accuracy"=>0.0, "deferred"=>0.0, "significant_change"=>"", "locations_in_payload"=>0, "device_id"=>"SM-S916B", "wifi"=>"Censored", "battery_state"=>"connectedNotCharging", "battery_level"=>79} permitted: true>} permitted: true>]} permitted: true>} permitted: true>, 1

sunstep avatar Feb 01 '25 17:02 sunstep

Update: I found the following in my sidekiq:

W, [2025-02-01T17:23:30.301807 #8] WARN -- : ArgumentError: 'connectedNotCharging' is not a valid battery_status

It shows an argument error as a warning. This is most likely the reason why it does not want to create the point. This argument error should be reported to my http client as response, instead of OK.

sunstep avatar Feb 01 '25 17:02 sunstep

@sunstep what are possible battery states your app can pass into dawarich?

Freika avatar Jul 26 '25 11:07 Freika

Thank you for getting back to this issue. Here is the documentation for battery state:

/// Indicates the current battery state.

enum BatteryState {
  /// The battery is fully charged.
  full,

  /// The battery is currently charging.
  charging,

  /// Device is connected to external power source, but not charging the battery.
  ///
  /// Usually happens when device has charge limit enabled and this limit is reached.
  /// Also, battery might be in this state if connected power source isn't powerful enough to charge the battery.
  ///
  /// Available on Android, MacOS and Linux platforms only.
  connectedNotCharging,

  /// The battery is currently losing energy.
  discharging,

  /// The state of the battery is unknown.
  unknown;
}

Supporting the above enum values would allow my android app to support the android battery state values natively, without requiring to adjust the state in certain cases. I believe connectedNotCharging is the only one that is not supported by Dawarich currently.

By the way, of course the /overland/batches endpoint is not really relevant anymore for me, but it still counts for the /points endpoint

sunstep avatar Jul 26 '25 21:07 sunstep

I suggest you sending statuses in snake_case (connected_not_charging), and I'll add it and discharging state in the next release

Freika avatar Jul 27 '25 19:07 Freika

Thank you, I'll take it into account in the next update.

sunstep avatar Jul 27 '25 19:07 sunstep