Skip to content
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

fix: attribute loss when updating entity #1667

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

Conversation

GuillaumeOd
Copy link

related to #1660

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

As a commented here #1660 (comment) a test case about his issue is needed.

@fgalan
Copy link
Member

fgalan commented Nov 6, 2024

@GuillaumeOd please have a look to documentation in the case it has to change to describe the new behaviour with this PR.

@AlvaroVega
Copy link
Member

This PR is reverting this commit: 166f771

@GuillaumeOdile GuillaumeOdile force-pushed the fix/attribute-loss-when-updating-a-device branch from 6c34980 to d9e6fdf Compare November 14, 2024 15:15
@fgalan
Copy link
Member

fgalan commented Nov 22, 2024

@GuillaumeOd not sure if I'm getting the point...

You want to send measures like this:

{
  "fireDetectionThreshold": 10
}

and get corresponding attribute at CB this way:

                    "fireDetectionThreshold": {
                        type: "Number",
                        value: 10
                    }

is my interpretation correct?

@AlvaroVega AlvaroVega self-requested a review November 22, 2024 13:41
@GuillaumeOd
Copy link
Author

GuillaumeOd commented Nov 22, 2024

@fgalan Yes, exactly! Regarding the type, I'm not entirely sure if it should be Number or Text, but your interpretation seems spot on.

Could you try running the test locally without my fix to confirm it fails as expected?

@fgalan
Copy link
Member

fgalan commented Nov 22, 2024

@fgalan Yes, exactly! Regarding the type, I'm not entirely sure if it should be Number or Text, but your interpretation seems spot on.

In that case, why don't use at provisioning time

                            {
                                name: 'fireDetectionThreshold',
                                type: 'Number'
                            }

instead of

                            {
                                object_id: 'f_dt',
                                name: 'fireDetectionThreshold',
                                type: 'Number'
                            }

?

@tzzed
Copy link

tzzed commented Jan 17, 2025

@fgalan we already send data like this

{
    "fireDetectionThreshold": 10
}

or by using the oid:

{
    "f_t": 10
}

since the version 4.0 it does not work. So after the fix of @GuillaumeOd both works. Why did you change this functionnality?
We really need this fix? In case of devices that use oid the iot agent does not support it anymore.
You can see the test of @GuillaumeOd and try it without the fix.
Thank you again.

@fgalan
Copy link
Member

fgalan commented Jan 22, 2025

@fgalan we already send data like this

{
    "fireDetectionThreshold": 10
}

or by using the oid:

{
    "f_t": 10
}

since the version 4.0 it does not work. So after the fix of @GuillaumeOd both works. Why did you change this functionnality? We really need this fix? In case of devices that use oid the iot agent does not support it anymore. You can see the test of @GuillaumeOd and try it without the fix. Thank you again.

So, you mean that you have some devices that are sending like "f_t": 10 and at the same time you have some other devices that are sending like "fireDetectionThreshold": 10.

Is my understanding correct?

@tzzed
Copy link

tzzed commented Jan 22, 2025

Yes exactly. Somes use object_id and othes use the name.

@fgalan
Copy link
Member

fgalan commented Jan 22, 2025

OK. Much clearer now :) Thanks!

since the version 4.0 it does not work. So after the fix of @GuillaumeOd both works. Why did you change this functionnality? We really need this fix? In case of devices that use oid the iot agent does not support it anymore. You can see the test of @GuillaumeOd and try it without the fix. Thank you again.

It was not an intentional change (but not within our use cases, so it was not noticed by us).

Thus, the PR is ok, as long as:

  1. Merge master on this PR's branch to check all test keep working
  2. This patch on the library has to be checked with IOTA-JSON, IOTA-UL and IOTA-Manager unit test. I'll take care of it, once you complete step 1

What do you think?

@tzzed
Copy link

tzzed commented Jan 22, 2025

Great thank you! that's work, @GuillaumeOd it's in your hands.

@AlvaroVega AlvaroVega requested review from fgalan and removed request for AlvaroVega January 23, 2025 08:08
@GuillaumeOd
Copy link
Author

thanks @fgalan ! I just merged master to the branch. Please notice that the test I added was failing because of a type error. The expected is:

fireDetectionThreshold: { type: 'Number', value: 10 }

but for some reason we receive:

fireDetectionThreshold: { type: 'Text', value: 10 }

Is this the normal behavior ?

@fgalan
Copy link
Member

fgalan commented Jan 23, 2025

thanks @fgalan ! I just merged master to the branch. Please notice that the test I added was failing because of a type error. The expected is:

fireDetectionThreshold: { type: 'Number', value: 10 }

but for some reason we receive:

fireDetectionThreshold: { type: 'Text', value: 10 }

Is this the normal behavior ?

Given the provision is

                            {
                                object_id: 'f_dt',
                                name: 'fireDetectionThreshold',
                                type: 'Number'
                            }

I guess that the correct one is

fireDetectionThreshold: { type: 'Number', value: 10 }

@AlvaroVega what do you think?

@AlvaroVega
Copy link
Member

@AlvaroVega what do you think?

I think both branch and test were aligned.

@GuillaumeOd
Copy link
Author

@AlvaroVega what do you think?

I think both branch and test were aligned.

The question is, what should be the expected payload to the test I added ? It appears that we receive a type Text but as my knowledge is limited concerning the type question in iotagent-node-lib I don't know if it should be Number for this case

@fgalan
Copy link
Member

fgalan commented Jan 24, 2025

@AlvaroVega what do you think?

I think both branch and test were aligned.

The question is, what should be the expected payload to the test I added ? It appears that we receive a type Text but as my knowledge is limited concerning the type question in iotagent-node-lib I don't know if it should be Number for this case

It should be Number, as it is in the current test. Thus, I guess that the code needs some extra fix to align with the tests.

@GuillaumeOd
Copy link
Author

@AlvaroVega what do you think?

I think both branch and test were aligned.

The question is, what should be the expected payload to the test I added ? It appears that we receive a type Text but as my knowledge is limited concerning the type question in iotagent-node-lib I don't know if it should be Number for this case

It should be Number, as it is in the current test. Thus, I guess that the code needs some extra fix to align with the tests.

I agree, shall I investigate it by myself ? The link with my PR is unlikely but I can try to find the fix and add it to this PR

@fgalan
Copy link
Member

fgalan commented Jan 24, 2025

I agree, shall I investigate it by myself ? The link with my PR is unlikely but I can try to find the fix and add it to this PR

It would be great :)

@AlvaroVega AlvaroVega dismissed their stale review January 24, 2025 09:25

not apply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants