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

Version option deprecation fix and replacements #1803

Merged
merged 13 commits into from Oct 15, 2020
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -8,12 +8,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Backward Compatibility Breaks
### Added
* Ability to specify the type of authentication manually by the `auth_type` parameter (in the client class config) was added (allowed values are `basic, digest, gssnegotiate, ntlm`)
* Added `if_seq_no` / `if_primary_term` to replace `version` for [optimistic concurrency control](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html)
* Added `Elastica\Aggregation\PercentilesBucket` aggregation [#1806](https://github.com/ruflin/Elastica/pull/1806)
### Changed
* Allow `string` such as `wait_for` to be passed to `AbstractUpdateAction::setRefresh` [#1791](https://github.com/ruflin/Elastica/pull/1791)
* Changed the return type of `AbstractUpdateAction::getRefresh` to `boolean|string` [#1791](https://github.com/ruflin/Elastica/pull/1791)
### Deprecated
* Deprecated Match query class and introduced MatchQuery instead for PHP 8.0 compatibility reason [#1799](https://github.com/ruflin/Elastica/pull/1799)
* Deprecated `version`/`version_type` options [(deprecated in `6.7.0`)](https://www.elastic.co/guide/en/elasticsearch/reference/6.8/docs-update.html) and added `if_seq_no` / `if_primary_term` that replaced it
### Removed
### Fixed
* fixed issue [1789](https://github.com/ruflin/Elastica/issues/1789)
Expand Down
93 changes: 71 additions & 22 deletions src/AbstractUpdateAction.php
Expand Up @@ -65,66 +65,115 @@ public function getIndex()
}

/**
* Sets the version of a document for use with optimistic concurrency control.
* Sets the version parameters of a document for use with optimistic concurrency control.
*
* @param int $version Document version
* @return $this
*/
public function setVersionParams(array $responseData): self
Copy link
Owner

Choose a reason for hiding this comment

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

Do we still need this method or could we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its still used to set the document version on GET

{
if (isset($responseData['_version'])) {
$this->setVersion($responseData['_version']);
}

if (isset($data['_seq_no'])) {
$this->setSequenceNumber($responseData['_seq_no']);
}

if (isset($data['_primary_term'])) {
$this->setPrimaryTerm($responseData['_primary_term']);
}

return $this;
}

/**
* Sets the sequence number of a document for use with optimistic concurrency control.
*
* @param int $number Sequence Number
*
* @return $this
*
* @see https://www.elastic.co/blog/versioning
* @see https://www.elastic.co/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html
*/
public function setVersion($version)
public function setSequenceNumber(int $number): self
{
return $this->setParam('version', (int) $version);
return $this->setParam('if_seq_no', $number);
}

/**
* Returns document version.
*
* @return int|string Document version
* @return int Document version
*/
public function getVersion()
public function getSequenceNumber(): int
{
return $this->getParam('version');
return $this->getParam('if_seq_no');
}

public function hasSequenceNumber(): bool
{
return $this->hasParam('if_seq_no');
}

/**
* @return bool
* Sets the primary term of a document for use with optimistic concurrency control.
*
* @param int $term Primary Term
*
* @return $this
*
* @see https://www.elastic.co/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html
*/
public function hasVersion()
public function setPrimaryTerm(int $term): self
{
return $this->hasParam('version');
return $this->setParam('if_primary_term', $term);
}

/**
* Returns document version.
*
* @return int Document version
*/
public function getPrimaryTerm(): int
{
return $this->getParam('if_primary_term');
}

public function hasPrimaryTerm(): bool
{
return $this->hasParam('if_primary_term');
}

/**
* Sets the version_type of a document
* Default in ES is internal, but you can set to external to use custom versioning.
* Sets the version of a document.
*
* @param string $versionType Document version type
* @param int $version Document version
*
* @return $this
*
* @see https://www.elastic.co/blog/versioning
*/
public function setVersionType($versionType)
public function setVersion($version)
{
return $this->setParam('version_type', $versionType);
return $this->setParam('version', (int) $version);
}

/**
* Returns document version type.
* Returns document version.
*
* @return int|string Document version type
* @return int|string Document version
*/
public function getVersionType()
public function getVersion()
{
return $this->getParam('version_type');
return $this->getParam('version');
}

/**
* @return bool
*/
public function hasVersionType()
public function hasVersion()
{
return $this->hasParam('version_type');
return $this->hasParam('version');
}

/**
Expand Down
4 changes: 1 addition & 3 deletions src/Bulk.php
Expand Up @@ -323,9 +323,7 @@ protected function _processResponse(Response $response): ResponseSet
if (!$data->hasId() && isset($bulkResponseData['_id'])) {
$data->setId($bulkResponseData['_id']);
}
if (isset($bulkResponseData['_version'])) {
$data->setVersion($bulkResponseData['_version']);
}
$data->setVersionParams($bulkResponseData);
Copy link
Owner

Choose a reason for hiding this comment

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

Now I see where we need this. Is there a better way to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure there's a better way, I was trying to avoid to copy/paste code

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think copy/paste in this case is fine, adding another public method just for avoiding calling each setter doesn't bring so much value.

}
}

Expand Down
17 changes: 6 additions & 11 deletions src/Client.php
Expand Up @@ -277,15 +277,13 @@ public function updateDocument($id, $data, $index, array $options = []): Respons

$docOptions = $data->getOptions(
[
'version',
'version_type',
Comment on lines -280 to -281
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems the right fix as this not documented anymore: https://www.elastic.co/guide/en/elasticsearch/reference/7.x/docs-update.html

'routing',
'percolate',
'parent',
'retry_on_conflict',
'consistency',
'replication',
'parent',
'percolate',
'refresh',
'replication',
'retry_on_conflict',
'routing',
'timeout',
]
);
Expand All @@ -310,10 +308,7 @@ public function updateDocument($id, $data, $index, array $options = []): Respons
&& $data instanceof Document
&& ($data->isAutoPopulate() || $this->getConfigValue(['document', 'autoPopulate'], false))
) {
$responseData = $response->getData();
if (isset($responseData['_version'])) {
$data->setVersion($responseData['_version']);
Comment on lines -314 to -315
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

}
$data->setVersionParams($response->getData());
}

return $response;
Expand Down
10 changes: 4 additions & 6 deletions src/Index.php
Expand Up @@ -215,9 +215,7 @@ public function addDocument(Document $doc): Response
if (isset($data['_id']) && !$doc->hasId()) {
$doc->setId($data['_id']);
}
if (isset($data['_version'])) {
$doc->setVersion($data['_version']);
Comment on lines -218 to -219
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

}
$doc->setVersionParams($data);
}

return $response;
Expand Down Expand Up @@ -272,10 +270,10 @@ public function getDocument($id, array $options = []): Document
$data = [];
}

$document = new Document($id, $data, $this->getName());
$document->setVersion($result['_version']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this should be removed finally. As it's still documented as returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll put back setVersion, just remove it for a document update

$doc = new Document($id, $data, $this->getName());
$doc->setVersionParams($data);

return $document;
return $doc;
}

/**
Expand Down