Olen / home-assistant-openplantbook

Integration to search and fetch data from Openplantbook.io
GNU General Public License v3.0
131 stars 5 forks source link

Error 400 Bad Request for PID in OpenPlantBook #20

Open ChristophCaina opened 6 months ago

ChristophCaina commented 6 months ago

Since the update to HA 2024.3.0 I am getting the following errors in the logs:

Dieser Fehler wurde von einer benutzerdefinierten Integration verursacht

Logger: openplantbook_sdk.sdk
Quelle: custom_components/openplantbook/uploader.py:105
Integration: OpenPlantbook (Dokumentation, Probleme)
Erstmals aufgetreten: 13:42:48 (4 Vorkommnisse)
Zuletzt protokolliert: 13:42:59

400 Bad Request: {'type': 'validation_error', 'errors': [{'code': 'invalid_pid', 'detail': "'Sansevieria trifasciata Prain var. laurentii' is unknown plant. This value has been neither found in Plant nor in User-Plant", 'attr': 'pid'}]}
400 Bad Request: {'type': 'validation_error', 'errors': [{'code': 'invalid_pid', 'detail': "'Nerium oleander' is unknown plant. This value has been neither found in Plant nor in User-Plant", 'attr': 'pid'}]}
400 Bad Request: {'type': 'validation_error', 'errors': [{'code': 'invalid_pid', 'detail': "'Citrus limon' is unknown plant. This value has been neither found in Plant nor in User-Plant", 'attr': 'pid'}]}
400 Bad Request: {'type': 'validation_error', 'errors': [{'code': 'invalid_pid', 'detail': "'Alocasia reginula' is unknown plant. This value has been neither found in Plant nor in User-Plant", 'attr': 'pid'}]}
Dieser Fehler wurde von einer benutzerdefinierten Integration verursacht

Logger: custom_components.openplantbook.uploader
Quelle: custom_components/openplantbook/uploader.py:112
Integration: OpenPlantbook (Dokumentation, Probleme)
Erstmals aufgetreten: 13:42:48 (4 Vorkommnisse)
Zuletzt protokolliert: 13:42:59

Unable to register Plant-instance: {'0c971b8b71bdeaf8b05693facb2d78b8': 'Sansevieria trifasciata Prain var. laurentii'} due to Exception:
Unable to register Plant-instance: {'4fee6c91d3823abcb09b9aa89a318eb5': 'Nerium oleander'} due to Exception:
Unable to register Plant-instance: {'a6be0f148e0e87e4465ef1e09d18341f': 'Citrus limon'} due to Exception:
Unable to register Plant-instance: {'5627817b47231b2ff30dae2cd0025a13': 'Alocasia reginula'} due to Exception:

It appears, that the plants are available in OpenPlantBook (example Sanseveria trifasciata prain var. laurentii) image

Olen commented 6 months ago

@slaxor505 Care to have a look?

slaxor505 commented 6 months ago

I'm investigating and trying to reproduce the issue.

slaxor505 commented 6 months ago

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. Moreover, it is only some plants for the particular user which have this display_pid issue but some plants are normal as I see they are being uploaded normally on the server end.

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.

    async def async_step_limits(self, user_input=None):
        """Handle max/min values"""

        plant_helper = PlantHelper(self.hass)
        if user_input is not None:
            _LOGGER.debug("User Input %s", user_input)

[skipped]

                self.plant_info[OPB_DISPLAY_PID] = user_input.get(OPB_DISPLAY_PID)
                if not self.plant_info[ATTR_SPECIES]:
                    self.plant_info[ATTR_SPECIES] = self.plant_info[OPB_DISPLAY_PID]

What this one for and what would be a condition when self.plant_info[ATTR_SPECIES] is not defined? I do need to have PID in the config/state entry to do sensor uploading.

Do you have any ideas what is the best way to address it?

ChristophCaina commented 6 months ago

Hi @slaxor505, Sorry for the delayed reply ... :-(

If still needed, I can get some more logs - but it will probably take some time...

Maybe, the information during setup might help? When I reconfigure the plant, I have those options:

image

There is the species - and the "displayed species name"... The species was copied from open Plant book - if you search for the plant, it will be shown with upper - and lower letters (display name) - the PID in the URL is completely with lower letters - and escaped white-spaces. https://open.plantbook.io/ui/plant-details/sansevieria%20trifasciata%20prain%20var.%20laurentii/

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?

slaxor505 commented 6 months ago

Hi @ChristophCaina

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.

Olen commented 6 months ago

Hi,

The code in question just make sure the field is filled out if the species was not set when the plant was created. Not everyone uses OPB, and not everyone care to spell the species exactly as in OPB. So if I just fill out "Chili" as species, that value will be inserted into multple fields in the entry.

slaxor505 commented 6 months ago

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.

Olen commented 5 months ago

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). But it is a text field, so the user can type whatever in the field.

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.

slaxor505 commented 5 months ago

@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.

ChristophCaina commented 5 months ago

Oh... I honestly don't remember. It is likely that I just copied the displayed name to the speciem field at some point, but 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. Let's see, if this will remove the error on my side - and then, I think - the field "speciem" should just convert user input into lower case letters, if this should work for both (fetching the information from OpenPlantBook and uploading sensor data to it)

Olen commented 5 months ago

Well, It could be clearer the "species" in the Options must match the "pid" in Openplantbook if you use OPB. If you don't use OPB, it does not matter.

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.

Olen commented 5 months ago

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.

slaxor505 commented 5 months ago

@ChristophCaina How did it go? Has it helped?

I think - the field "speciem" should just convert user input into lower case letters, if this should work for both (fetching the information from OpenPlantBook and uploading sensor data to it)

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.

slaxor505 commented 5 months ago

@Olen Thank you for that.

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

In the doc you say: The species will have to be entered exactly as the "pid" or "scientific species" in OpenPlantbook (including any punctations etc.). If the species is found in OpenPlantbook, the thresholds are updated to the new values.

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.

If no species are found in OpenPlantbook, the thresholds and image will be retained with their current values.

Perhaps showing that "not found in OPB" would be a good indication of the problem so less troubles for users further down the line.

slaxor505 commented 5 months ago

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

ChristophCaina commented 5 months ago

@ChristophCaina How did it go? Has it helped?

I think - the field "speciem" should just convert user input into lower case letters, if this should work for both (fetching the information from OpenPlantBook and uploading sensor data to it)

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.

Honestly, I haven't checked it immediately after the change as I was pretty busy with other stuff ^^ And just tonight it seems that my HA server crashed...

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.

Olen commented 5 months ago

In the doc you say: The species will have to be entered exactly as the "pid" or "scientific species" in OpenPlantbook (including any punctations etc.). If the species is found in OpenPlantbook, the thresholds are updated to the new values.

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?

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. I removed the reference to "scientific species" from the README now to make it clear that it needs to be the "pid".

In the configuration, there is no "search", just a "get", so it needs to be the "pid".

It simply runs this function: https://github.com/Olen/homeassistant-plant/blob/79ef86e95cda7c658f1c31b1f3bef22c03d59338/custom_components/plant/plant_helpers.py#L159

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 openplantbook_get functons in the other hand (which is called from the above function): https://github.com/Olen/homeassistant-plant/blob/79ef86e95cda7c658f1c31b1f3bef22c03d59338/custom_components/plant/plant_helpers.py#L126

Is supposed to log and create a notification if nothing is found in OPB: https://github.com/Olen/homeassistant-plant/blob/79ef86e95cda7c658f1c31b1f3bef22c03d59338/custom_components/plant/plant_helpers.py#L151-L156

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 openplantbook_get() function.

So maybe the openplantbook_get() (and probably also _search()?) needs a fix to understand the updated response from OPB.

Olen commented 5 months ago

Just another observation.

The openplantbook_get-function in the plant-component just runs the service-call from this integration. Tat service call runs this function: https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/__init__.py#L115

That function again calls async_plant_detail_get https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/__init__.py#L134

from the openplantbook_sdk.

So really, this function should maybe raise an exception if nothing is found, so the exception can be caught like this one: https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/__init__.py#L137

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.

slaxor505 commented 5 months ago

from the openplantbook_sdk.

So really, this function should maybe raise an exception if nothing is found, so the exception can be caught like this one:

https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/__init__.py#L137

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:

https://github.com/slaxor505/openplantbook-sdk-py/blob/b608eeaf4a519f229ca7b93fbd14f69fce45e4bb/openplantbook_sdk/sdk.py#L117

Olen commented 5 months ago

Hi,

It looks from the OP that the problem arises here:

https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/uploader.py#L112

So it is the async_plant_instance_register() function that raises an exception when the pid is not found.

I thought this had something to do with creating the plant in the first place, or with changing the species.

slaxor505 commented 5 months ago

@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.

https://github.com/Olen/home-assistant-openplantbook/blob/8f526ef3117af8ca2f90bc2ef1438fa020a74ef8/custom_components/openplantbook/uploader.py#L96

The requirement to register a sensor is that the sensor needs to be bundled with the valid PID.

Olen commented 5 months ago

I believe the best thing you can do here is to update uploader.py to catch the exception from async_plant_instance_register() and log the error and create a notification, like what is done in the config-flow for plants:

https://github.com/Olen/homeassistant-plant/blob/79ef86e95cda7c658f1c31b1f3bef22c03d59338/custom_components/plant/plant_helpers.py#L151-L156

slaxor505 commented 4 months ago

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. I'm going to make a dirty workaround within this integration where I will be searching for display_pid and it is found then I will retry plant-instance registration with the correct pid.

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.

slaxor505 commented 3 months ago

I've implemented workaround in beta5. @Olen please merge PR.