🐛 Fix: Do not block on zero results from JSON endpoint.

[jbostoen]

Base information

Question	Answer
Related to a SourceForge thread / Another PR / Combodo ticket?	-
Type of change?	Bug fix / Enhancement

Symptom (bug) / Objective (enhancement)

I consider it a bug - you might consider it an enhancement.

For a custom collector, a JSON API may return an empty array (no results); even if the API call is successful.

This means the Prepare method results in false, and this result is also returned by Collect.

itop-data-collector-base/core/collector.class.inc.php

Lines 548 to 582 in f8077b5

	public function Collect($iMaxChunkSize = 0)
	{
	Utils::Log(LOG_INFO, get_class($this)." beginning of data collection...");
	try {
	$bResult = $this->Prepare();
	if ($bResult) {
	$idx = 0;
	$aHeaders = null;
	while ($aRow = $this->Fetch()) {
	if ($aHeaders == null) {
	// Check that the row names are consistent with the definition of the task
	$aHeaders = array_keys($aRow);
	}
	if (($idx == 0) || (($iMaxChunkSize > 0) && (($idx % $iMaxChunkSize) == 0))) {
	$this->NextCSVFile();
	$this->AddHeader($aHeaders);
	}
	$this->AddRow($aRow);
	$idx++;
	}
	$this->Cleanup();
	Utils::Log(LOG_INFO, get_class($this)." end of data collection.");
	} else {
	Utils::Log(LOG_ERR, get_class($this)."::Prepare() returned false");
	}
	} catch (Exception $e) {
	$bResult = false;
	Utils::Log(LOG_ERR, get_class($this)."::Collect() got an exception: ".$e->getMessage());
	}
	return $bResult;
	}

Now, when Collect returns false, this may break the orchestration (and thus prevents any subsequent and possibly non-related/non-dependent collectors to be executed).

itop-data-collector-base/core/orchestrator.class.inc.php

Lines 235 to 249 in f8077b5

	public function Collect($aCollectors, $iMaxChunkSize, $bCollectOnly)
	{
	$bResult = true;
	/** @var \Collector $oCollector */
	foreach ($aCollectors as $oCollector) {
	Utils::SetCollector($oCollector, "Collect");
	$bResult = $oCollector->Collect($iMaxChunkSize, $bCollectOnly);
	if (!$bResult) {
	break;
	}
	}
	Utils::SetCollector(null);
	return $bResult;
	}

Reproduction procedure (bug)

• Create 2 JSON collector classes.
• Orchestrate them.
• Have them poll an API endpoint that is valid but just does not return any objjects.
• See how the second one is never even executed.

Cause (bug)

See above.

Proposed solution (bug and enhancement)

I raise this PR for visibility, and for the technical explanation.
It offers one solution; where the developer of the collector decides whether no results should mean a full stop of processing.

It can of course be discussed if this should be an XML setting instead (my two cents: it should be in the implementation, it's not really a customization option); or where to catch it.

Perhaps the orchestrator logic should be addressed instead and still continue?
Or the setting could be on the orchestrator?

I believe the way I propose now, is probably the most flexible one; allow specifying it per collector.

Then of course: what should the default value be?
Don't stop processing seems to be the proper one; but this does mean it's a change from before?

Checklist before requesting a review

• I have performed a self-review of my code, and that it's compliant with Combodo's guidelines
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't
• I have made sure the PR is clear and detailled enough so anyone can understand the real purpose without digging in the code

Checklist of things to do before PR is ready to merge

Decide on preferred solution.

[Hipska]

To me it looks like it should be a static instead. The child class can then override the value. The default value should be the current behavior.