-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Error 400 Bad Request for PID in OpenPlantBook #20
Comments
@slaxor505 Care to have a look? |
I'm investigating and trying to reproduce the issue. |
Hey @Olen. It is a bit odd. In the uploader.py I take plant's PID as: opb_pid = plant_device_state[plant_entity_id][0].attributes["species_original"] The species_original refers to "species" attribute value which is supposed to be PID. Although, in this particular case it is "display_pid" as PID is always lowercase and here it is not lower-case as it can be seen in the logs. Do you have any idea how display_pid could appear in the spieces attribute: Sansevieria trifasciata Prain var. laurentii. This is the problem because PID is case sensitive and OPB cannot find the plant in DB. I've dug dipper and I see that in plant component during plant instance setup you have this condition which is likely cause the issue.
What this one for and what would be a condition when self.plant_info[ATTR_SPECIES] is not defined? Do you have any ideas what is the best way to address it? |
Hi @slaxor505, If still needed, I can get some more logs - but it will probably take some time... Maybe, the information during setup might help? There is the species - and the "displayed species name"... The Data-Upload to openPlant Book is still beta - so it might just be required to change any user input to lower letters - and I think this should be fine then? |
Your response was actually pretty quick so no worries :) I did not realize that I posted message twice. It is all good I have better understanding on what's going on so no logs necessary at this stage. To answer to your question if the plants need to be re-added with lower-case, it may be necessary in your particular case but not before we fix the root cause of the issue. This needs to be handled by components. We just need to work out with @Olen what's the best way to address it. The issue is plant_component (or maybe something else) inconsistently set "species" attribute to either "pid" or "display_pid" in its configuration and this is why openplantbook_component/API cannot find the correct plant in DB. Let us work it through. If it is not a big deal you can try and re-add these problematic plants to HASS but again there is an issue and we gonna fix it. |
Hi, The code in question just make sure the field is filled out if the species was not set when the plant was created. |
Got it. Make sense, but how in this particular case where OPB is being used but display_pid appeared in the species instead of PID I cannot get. It should not be happening, should it. |
tl;dr:You need to use the "pid" (all lowercase) in the "species" field when you modify the configuration So, this is the flow for creating a new plant:OPB_DISPLAY_PID is "display_pid" whereas ATTR_SPECIES is "species" When you configure a plant, you add a name and a "species" (ATTR_SPECIES) as a text field. We copy that value into the field "search_for". We then search OPB for that string. If nothing is found we go directly to the "limits" step. If we find something in OPB, we change the "species" field to a dropdown and let the user select the correct species. In the limits-step, the user can change or confirm all the data from OPB (or select to go back to the previous step by unselecting the "Correct plant" checkbox). AFTER the user has filled inn (or confirmed) the limits etc. we do the check you have found, and will use the "display_pid" as "species" if "species" is not set. The only places "species" is modified is when the user first type in something, and when the field is changed to a dropdown with the results from OPB. And the only time this field can be empty is if the user does not write anything, OR if OPB somehow returns an empty value for the field, so the dropdown is populated with an empty value for one or more of the results. When it comes to reconfigure, the process is slightly different:Here, the "species" field is pre-filled with the current value (all in lowercase etc). In THIS case, we just search OPB for whatever the user is typing into that field, so that field needs to be a "pid" from OPB. |
@ChristophCaina Have you modified these plants after they were initially configured? Or has done something unusual to them? I want to try and reproduce what @Olen described. I still think this issue should be addressed in the code but dunno yet what's the best way to do it. |
Oh... I honestly don't remember. I now changed those to be all lower case letters to match the other sensors that were not affected from this error. |
Well, The problem is that there is not much flexibility in the form, so there is no easy way to explain that in a good way on the actual options page. |
I just looked through the (quite extensive) README for the plant integration, and found that I have documented this behaviour already here: https://github.com/Olen/homeassistant-plant?tab=readme-ov-file#changing-the-species--refreshing-data I realise not everyone reads the entire README, but there is quite a lot of information there that might be useful if something is not working as expected. |
@ChristophCaina How did it go? Has it helped?
Unfortunately, it is not a rule that pid = lowercased display_pid. If I just lowercase it they there is no 100% guarantee that it will work. I'll think what's the best way to address it. I'm going to monitor similar 400 error and see if it is frequent case or yours was just one off. |
@Olen Thank you for that.
In the doc you say: So it can be either display_pid or pid so it looks like that you gonna make a search anyway in OPB after this form is submitted. Why you could not take pid at that stage and just override everything in species attribute? What happens if it does not match - error occurs? It looks like nothing happens so a user is not aware that there is a problem.
Perhaps showing that "not found in OPB" would be a good indication of the problem so less troubles for users further down the line. |
Another inconsistency is that I don't show PID to a user in OPB UI so I gonna fix it. I only show "Scientific name" which is display_pid |
Honestly, I haven't checked it immediately after the change as I was pretty busy with other stuff ^^ I had a quick look through the logs, but could not find any issue related to the integration - so it seems the change had helped. |
Then that is a misunderstanding from my side. Maybe from an early version of OPB? I thought "pid" and "scientific species" was the same, just different labels at different locations in OPB, while "display species" was something else. In the configuration, there is no "search", just a "get", so it needs to be the "pid". It simply runs this function: With the old values as default parameters. and as that function is indifferent to whether OPB is configured or not, it does not really know of OPB is configured and does not find anything, or if OPB is not set up at all. The Is supposed to log and create a notification if nothing is found in OPB: If this does not work, it means that either there is a bug there somewhere, or (as it seems from the OP) that OPB does not really return an error, but a valid repsonse, which is not interpreted as an error by the So maybe the |
Just another observation. The
That function again calls
from the So really, this function should maybe raise an exception if nothing is found, so the exception can be caught like this one:
Or at least it should return None if nothing is found, so the rest of the chain of functions are not fooled into believeing a result was returned. |
It does return "None" if no pid found: |
Hi, It looks from the OP that the problem arises here:
So it is the I thought this had something to do with creating the plant in the first place, or with changing the species. |
@Olen yes, this is when the problem with "species" attribute surfaces. Because the value in the attribute is not a correct PID (it is display_pid rather in this particular case), the uploader cannot register sensor with corresponding correct plant. The uploader takes PID from a plant's species attribute.
The requirement to register a sensor is that the sensor needs to be bundled with the valid PID. |
I believe the best thing you can do here is to update |
I've added some more logging on server side and I see that it is pretty common that a faulty plant corresponds to a plant from OPB DB but species has been set to display_pid rather than pid which is a problem for sensor data uploader. But I still think that something needs to be done in the Plant component. By setting species to OPB's pid when the plant is being fetched. If the plant is not from OPB then it can be left as it is now. |
I've implemented workaround in beta5. @Olen please merge PR. |
Since the update to HA 2024.3.0 I am getting the following errors in the logs:
It appears, that the plants are available in OpenPlantBook (example Sanseveria trifasciata prain var. laurentii)
The text was updated successfully, but these errors were encountered: