ConnectedHumber / Air-Quality-Web

The web interface and JSON api for the ConnectedHumber Air Quality Monitoring Project.
https://sensors.connectedhumber.org/
Mozilla Public License 2.0
9 stars 4 forks source link

API action list-devices-near is not quite working - it returns only the same sensor #61

Closed bsimmo closed 4 years ago

bsimmo commented 4 years ago

Site is working, but the API list-devices-near is not quite working.

If I asked for 3 locations, I would get three before (as intended) Now I just get the same one (nearest) three times.

Example Python3 code

import json
from urllib import request, parse

api_url = "http://sensors.connectedhumber.org/beta/api.php?action="

no_of_locations = 3
my_location = ( 53.750, -0.395 )

def get_nearest_sensors( my_location, no_of_locations ):
    url = str( api_url ) + "list-devices-near&count=" + str( no_of_locations )
    data = parse.urlencode( {'latitude': my_location[0], 'longitude': my_location[1]} ).encode()
    req = request.Request( url, data=data )
    response = request.urlopen( req )
    return response

# ------- #
nearest_sensors = json.loads( get_nearest_sensors( my_location, no_of_locations ).read().decode('utf-8') )
print( nearest_sensors )

for line in nearest_sensors:
    print( line['id'], line['name'], "is", round( line['distance_actual']*0.000621, 1), "miles away" )
print("----------")

Currently returns

[{'id': 36, 'name': 'CHASW-24AD43-1', 'latitude': 53.75642593, 'longitude': -0.36844245, 'distance_calc': 1886.588853560329, 'last_seen': '2020-03-08 21:32:06', 'distance_actual': 1892.19}, {'id': 36, 'name': 'CHASW-24AD43-1', 'latitude': 53.75642593, 'longitude': -0.36844245, 'distance_calc': 1886.588853560329, 'last_seen': '2020-03-08 21:32:06', 'distance_actual': 1892.19}, {'id': 36, 'name': 'CHASW-24AD43-1', 'latitude': 53.75642593, 'longitude': -0.36844245, 'distance_calc': 1886.588853560329, 'last_seen': '2020-03-08 21:32:06', 'distance_actual': 1892.19}]
36 CHASW-24AD43-1 is 1.2 miles away
36 CHASW-24AD43-1 is 1.2 miles away
36 CHASW-24AD43-1 is 1.2 miles away

Also as an aside, I thought 'distance_calc': 1886.588853560329 field was being removed as 'distance_actual' is the correct geodesic distance.

Originally posted by @bsimmo in https://github.com/ConnectedHumber/Air-Quality-Web/issues/59#issuecomment-596255938

sbrl commented 4 years ago

Thanks for opening a new issue about this. Sorry I didn't reply, I've been busy (both with PhD work, and sorting out the disclaimer on here).

I'll take a look into this.

Edit: Do you have that as a curl command? That would be much easier for me to test with

sbrl commented 4 years ago

For my own reference the above can be represented with curl like so:

curl 'http://[::1]:40482/api.php?action=list-devices-near&count=5' --data "latitude=53.750" --data "longitude=-0.395" | jq .
sbrl commented 4 years ago

With some investigation, it would seem that the SQL query is at fault...... somehow. Here's the SQL query for that api call:

SELECT
devices.device_id AS id,
devices.device_name AS name,
devices.device_latitude AS latitude,
devices.device_longitude AS longitude,
ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon) AS distance_calc, devices.last_seen AS last_seen
FROM devices
JOIN readings ON readings.device_id = devices.device_id
WHERE devices.lat_lon IS NOT NULL
ORDER BY ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon)
LIMIT 5;

I'm somewhat baffled as to what it's suddenly generating duplicates. Thoughts?

/cc @BNNorman

BNNorman commented 4 years ago

duplicates normally arise if you don't use a right join

bsimmo commented 4 years ago

I know it once worked, we/you altered it. I then asked for last seen and the commit a678a89a was made. I cannot remember if I tested if that worked, or if that brought in the last problem (server error), that was then fixed.

Just checking you are using the current beta server as the fix was never release to the live server. At least for normal users.

as for time, same problem here. reporting things between everything else when I drop in and have a go at using it :-)

sbrl commented 4 years ago

Ah, I see @BNNorman. If I change the query to this though:

SELECT
devices.device_id AS id,
devices.device_name AS name,
devices.device_latitude AS latitude,
devices.device_longitude AS longitude,
ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon) AS distance_calc, devices.last_seen AS last_seen
FROM devices
RIGHT JOIN readings ON readings.device_id = devices.device_id
WHERE devices.lat_lon IS NOT NULL
ORDER BY ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon)
LIMIT 5;

...I still get the same result:

+----+----------------+-------------+-------------+-------------------+---------------------+
| id | name           | latitude    | longitude   | distance_calc     | last_seen           |
+----+----------------+-------------+-------------+-------------------+---------------------+
| 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 |
| 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 |
| 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 |
| 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 |
| 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 |
+----+----------------+-------------+-------------+-------------------+---------------------+

...even though I'm specifying a right join explicitly.

@bsimmo: This is using a local development server that's running the latest from the dev branch.

Hrm. You're saying that this is a regression and it worked before? Thanks for that commit id - that;s really helpful for narrowing down what's gone wrong. I'm pretty sure the problematic change is in https://github.com/ConnectedHumber/Air-Quality-Web/commit/a678a89b952e2131a697cf12fc917c66b900af5d#diff-de7016366166fa0b532d03d624168e27R151-R176.

Thinking about it, I'm not even sure why we're joining there in the first place, since it would appear that the only time the readings table is referenced is in the join itself. If I do this instead:

SELECT
devices.device_id AS id,
devices.device_name AS name,
devices.device_latitude AS latitude,
devices.device_longitude AS longitude,
ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon) AS distance_calc, devices.last_seen AS last_seen
FROM devices
WHERE devices.lat_lon IS NOT NULL
ORDER BY ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon)
LIMIT 5;

...then I correctly get this:

+----+------------------+-------------+-------------+--------------------+---------------------+
| id | name             | latitude    | longitude   | distance_calc      | last_seen           |
+----+------------------+-------------+-------------+--------------------+---------------------+
| 36 | CHASW-24AD43-1   | 53.75642593 | -0.36844245 |  1886.588853560329 | 2020-05-01 22:15:46 |
| 32 | CHASW-56AB6D-1   |   53.767262 |   -0.403431 | 1997.8605170536607 | 2020-05-01 06:00:56 |
| 51 | CL-AW15CH6N      |  53.7432075 |  -0.3586957 | 2503.8574249443122 | 2020-03-12 23:00:00 |
| 35 | CHASR-6F3BD82C-1 |    53.75776 |   -0.436268 |  2847.053372632466 | 2020-05-01 22:17:15 |
| 48 | HCCSENSOR03      |   53.739378 | -0.34838408 | 3285.0856113502487 | 2020-05-01 22:16:51 |
+----+------------------+-------------+-------------+--------------------+---------------------+

...does this look right to everyone? Have I missed something? I can't remember what I was thinking at the time, and the commit message doesn't give anything away. I feel like I'm missing something.

If this altered SQL query looks ok, I'll go ahead and patch the SQL query by removing the JOIN altogether.

BNNorman commented 4 years ago

Good call I didnt query the need for a join at all. Result looks ok to me

On Fri, 1 May 2020, 23:20 Starbeamrainbowlabs, notifications@github.com wrote:

Ah, I see @BNNorman https://github.com/BNNorman. If I change the query to this though:

SELECTdevices.device_id AS id,devices.device_name AS name,devices.device_latitude AS latitude,devices.device_longitude AS longitude, ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon) AS distance_calc, devices.last_seen AS last_seenFROM devicesRIGHT JOIN readings ON readings.device_id = devices.device_idWHERE devices.lat_lon IS NOT NULLORDER BY ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon)LIMIT 5;

...I still get the same result:

+----+----------------+-------------+-------------+-------------------+---------------------+ | id | name | latitude | longitude | distance_calc | last_seen | +----+----------------+-------------+-------------+-------------------+---------------------+ | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 | | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 | | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 | | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 | | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:09:46 | +----+----------------+-------------+-------------+-------------------+---------------------+

...even though I'm specifying a right join explicitly.

@bsimmo https://github.com/bsimmo: This is using a local development server that's running the latest from the dev branch.

Hrm. You're saying that this is a regression and it worked before? Thanks for that commit id - that;s really helpful for narrowing down what's gone wrong. I'm pretty sure the problematic change is in a678a89

diff-de7016366166fa0b532d03d624168e27R151-R176

https://github.com/ConnectedHumber/Air-Quality-Web/commit/a678a89b952e2131a697cf12fc917c66b900af5d#diff-de7016366166fa0b532d03d624168e27R151-R176 .

Thinking about it, I'm not even sure why we're joining there in the first place, since it would appear that the only time the readings table is referenced is in the join itself. If I do this instead:

SELECTdevices.device_id AS id,devices.device_name AS name,devices.device_latitude AS latitude,devices.device_longitude AS longitude, ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon) AS distance_calc, devices.last_seen AS last_seenFROM devicesWHERE devices.lat_lon IS NOT NULLORDER BY ST_DISTANCE_SPHERE(POINT(53.750, -0.395), devices.lat_lon)LIMIT 5;

...then I correctly get this:

+----+------------------+-------------+-------------+--------------------+---------------------+ | id | name | latitude | longitude | distance_calc | last_seen | +----+------------------+-------------+-------------+--------------------+---------------------+ | 36 | CHASW-24AD43-1 | 53.75642593 | -0.36844245 | 1886.588853560329 | 2020-05-01 22:15:46 | | 32 | CHASW-56AB6D-1 | 53.767262 | -0.403431 | 1997.8605170536607 | 2020-05-01 06:00:56 | | 51 | CL-AW15CH6N | 53.7432075 | -0.3586957 | 2503.8574249443122 | 2020-03-12 23:00:00 | | 35 | CHASR-6F3BD82C-1 | 53.75776 | -0.436268 | 2847.053372632466 | 2020-05-01 22:17:15 | | 48 | HCCSENSOR03 | 53.739378 | -0.34838408 | 3285.0856113502487 | 2020-05-01 22:16:51 | +----+------------------+-------------+-------------+--------------------+---------------------+

...does this look right to everyone? Have I missed something? I can't remember what I was thinking at the time, and the commit message doesn't give anything away. I feel like I'm missing something.

If this altered SQL query looks ok, I'll go ahead and patch the SQL query by removing the JOIN altogether.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ConnectedHumber/Air-Quality-Web/issues/61#issuecomment-622590724, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADY5NXMO2AYPGHYNS56JCQDRPNDMTANCNFSM4MXAIDZQ .

sbrl commented 4 years ago

Fixed - at least for me. Should land in beta in ~20 mins