Skip to content

N°5139 - Let data that is null also be null at the other side-fix #52

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

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

Conversation

odain-cbd
Copy link
Contributor

@odain-cbd odain-cbd commented May 15, 2024

Current PR intends to enhance "Reset or untouch" option (nullified configuration) to work also for empty string ("") fields values.

During data collection, fields can have a null value. during iTop object synchronization you can decide either

  • to reset the object field value on iTop side (during synchronization) and do nothing special in iTop configuration.
  • to update the field to a default value configured on collector side. use "default" configuration section for that purpose
  • to keep its previous value by leaving it untouch and sending '' to iTop synchonization. you have to configure collector with a nullified section.

we propose to have same "untouch" behaviour for empty string values collected. as a matter of fact null cannot be collected for some type of collectors (like CSV). which make the "nullify" feature unusable... seems like a bug to me.

  • cf documentation :

https://wiki.combodo.com/doku.php?id=extensions:itop-data-collector-base#reset_or_untouch

  • cf PR that brought the feature:

#7

@odain-cbd odain-cbd added the bug Something isn't working label May 15, 2024
@odain-cbd odain-cbd self-assigned this May 15, 2024
@odain-cbd odain-cbd requested a review from v-dumas May 15, 2024 13:46
@Hipska
Copy link
Collaborator

Hipska commented May 17, 2024

I'm only concerned on how to sync a NULL value into a nullable DECIMAL field in iTop. I think that is still not possible, even with these additions?

Edit: This is the correct link to the documentation: https://www.itophub.io/wiki/page?id=extensions:itop-data-collector-base#reset_or_untouch

@Molkobain
Copy link
Contributor

I'm only concerned on how to sync a NULL value into a nullable DECIMAL field in iTop. I think that is still not possible, even with these additions?

Edit: This is the correct link to the documentation: https://www.itophub.io/wiki/page?id=extensions:itop-data-collector-base#reset_or_untouch

Could we add a unit test on that to check and to ensure that we keep the intended behavior?

@Hipska
Copy link
Collaborator

Hipska commented Apr 18, 2025

Hi @odain-cbd, what is the status / progress on this PR?

Hipska and others added 11 commits April 23, 2025 10:00
To classify PRs
…hed from its home directory (#55)

* Make sure relative paths can be used when json collector is launched from outside its root directory

* Add test for jsonfile parameter defined with relative path

* Provider function must be static for PHP 8.2+
Suppress the PHP warning generated by file_get_contents when requested file is not found. The error is handled by the code.
* Log the trimmed output in debug mode when there are no errors

* Revert previous default behaviour

* Also fix the hardcoded default value
@odain-cbd
Copy link
Contributor Author

odain-cbd commented Apr 23, 2025

I'm only concerned on how to sync a NULL value into a nullable DECIMAL field in iTop. I think that is still not possible, even with these additions?
Edit: This is the correct link to the documentation: https://www.itophub.io/wiki/page?id=extensions:itop-data-collector-base#reset_or_untouch

Could we add a unit test on that to check and to ensure that we keep the intended behavior?

there is a test that covers nullify feature (testAttributeIsNullified).
cf https://github.com/Combodo/itop-data-collector-base/pull/52/files#diff-1c713ca97f4d38c6b13f253c21e666fb7a7051f40c2a4c3b59732fcea6d518b8

@odain-cbd
Copy link
Contributor Author

Hi @odain-cbd, what is the status / progress on this PR?

Hi Hipska,

current PR was on hold on my side. still waiting for reviewers to approve.

@Hipska
Copy link
Collaborator

Hipska commented Apr 23, 2025

That test doesn't actually test if the value on iTop becomes NULL after synchro. It only tests if resulting CSV contains <NULL>.

So question remains for very long time; How to synchronise a DECIMAL field to have the resulting value in iTop be NULL?

IMHO it has never been able to achieve this.

if (is_null($aRow[$sHeader]) && $this->AttributeIsNullified($sHeader)) {
if (strlen($aRow[$sHeader] ?? '')===0 && $this->AttributeIsNullified($sHeader)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty string is not the same as null.

Copy link
Contributor Author

@odain-cbd odain-cbd Apr 23, 2025

Choose a reason for hiding this comment

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

indeed yes.

untouched scenario:
for a field, either it is configured as "nullified", and both null or empty string will be sent as <NULL> during datasynchro, which will leave it with its previous value on iTop side (untouched).

reset scenario:
otherwise same field (either null or empty string) will be sent during datasynchro as empty field. which will reset field value to default one.

for some iTop attributes this usecase is supported. for some others it is not. at the end you can decide what to do from collector side (per attribute xml configuration).

Copy link
Collaborator

@Hipska Hipska Jun 3, 2025

Choose a reason for hiding this comment

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

No, The whole thing is to be able to set a field's value as NULL in the SQL database, not reset, not ignore!

So it seems it will still not work and Combodo still doesn't understand the needs.. 😢

So question remains for very long time; How to synchronise a DECIMAL field to have the resulting value in iTop be NULL?

IMHO it has never been able to achieve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants