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,11 +8,13 @@ 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)
### 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
78 changes: 76 additions & 2 deletions src/AbstractUpdateAction.php
Expand Up @@ -65,6 +65,71 @@ public function getIndex()
}

/**
* 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/guide/en/elasticsearch/reference/6.8/optimistic-concurrency-control.html
*/
public function setSequenceNumber($number)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setSequenceNumber($number)
public function setSequenceNumber(int $number)

{
return $this->setParam('if_seq_no', $number);
}

/**
* Returns document version.
*
* @return int|string Document version
*/
public function getSequenceNumber()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getSequenceNumber()
public function getSequenceNumber(): int

{
return $this->getParam('if_seq_no');
}

/**
* @return bool
*/
public function hasSequenceNumber()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function hasSequenceNumber()
public function hasSequenceNumber(): bool

{
return $this->hasParam('if_seq_no');
}

/**
* Sets the prmary term of a document for use with optimistic concurrency control.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Sets the prmary term of a document for use with optimistic concurrency control.
* 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 setPrimaryTerm($term)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setPrimaryTerm($term)
public function setPrimaryTerm(int $term)

{
return $this->setParam('if_primary_term', $term);
}

/**
* Returns document version.
*
* @return int|string Document version
*/
public function getPrimaryTerm()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function getPrimaryTerm()
public function getPrimaryTerm(): int

{
return $this->getParam('if_primary_term');
}

/**
* @return bool
*/
public function hasPrimaryTerm()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function hasPrimaryTerm()
public function hasPrimaryTerm(): bool

{
return $this->hasParam('if_primary_term');
}

/**
* @deprecated
* Sets the version of a document for use with optimistic concurrency control.
*
* @param int $version Document version
Expand All @@ -75,10 +140,13 @@ public function getIndex()
*/
public function setVersion($version)
{
return $this->setParam('version', (int) $version);
\trigger_error('Elastica\AbstractUpdateAction::setVersion is deprecated. Use Elastica\AbstractUpdateAction::setSequenceNumber and Elastica\AbstractUpdateAction::setPrimaryTerm instead.', E_USER_DEPRECATED);

return $this;
}

/**
* @deprecated
* Returns document version.
*
* @return int|string Document version
Expand All @@ -89,6 +157,7 @@ public function getVersion()
}

/**
* @deprecated
* @return bool
*/
public function hasVersion()
Expand All @@ -97,6 +166,7 @@ public function hasVersion()
}

/**
* @deprecated
* Sets the version_type of a document
* Default in ES is internal, but you can set to external to use custom versioning.
*
Expand All @@ -106,10 +176,13 @@ public function hasVersion()
*/
public function setVersionType($versionType)
{
return $this->setParam('version_type', $versionType);
\trigger_error('Elastica\AbstractUpdateAction::setVersionType is deprecated. Use Elastica\AbstractUpdateAction::setSequenceNumber and Elastica\AbstractUpdateAction::setPrimaryTerm instead.', E_USER_DEPRECATED);

return $this;
}

/**
* @deprecated
* Returns document version type.
*
* @return int|string Document version type
Expand All @@ -120,6 +193,7 @@ public function getVersionType()
}

/**
* @deprecated
* @return bool
*/
public function hasVersionType()
Expand Down
7 changes: 5 additions & 2 deletions src/Bulk.php
Expand Up @@ -323,8 +323,11 @@ protected function _processResponse(Response $response): ResponseSet
if (!$data->hasId() && isset($bulkResponseData['_id'])) {
$data->setId($bulkResponseData['_id']);
}
if (isset($bulkResponseData['_version'])) {
$data->setVersion($bulkResponseData['_version']);
if (isset($responseData['_seq_no'])) {
$data->setSequenceNumber($responseData['_seq_no']);
}
if (isset($responseData['_primary_term'])) {
$data->setPrimaryTerm($responseData['_primary_term']);
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions src/Client.php
Expand Up @@ -277,8 +277,8 @@ 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

'if_seq_no',
'if_primary_term',
'routing',
'percolate',
'parent',
Expand Down Expand Up @@ -311,8 +311,11 @@ public function updateDocument($id, $data, $index, array $options = []): Respons
&& ($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.

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

Expand Down
12 changes: 6 additions & 6 deletions src/Index.php
Expand Up @@ -215,8 +215,11 @@ 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.

if (isset($responseData['_seq_no'])) {
$doc->setSequenceNumber($responseData['_seq_no']);
}
if (isset($responseData['_primary_term'])) {
$doc->setPrimaryTerm($responseData['_primary_term']);
}
}

Expand Down Expand Up @@ -272,10 +275,7 @@ 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


return $document;
return new Document($id, $data, $this->getName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably add the seq_no and primary_term to document too as it's documented as returned: https://www.elastic.co/guide/en/elasticsearch/reference/7.9/docs-get.html#docs-get-api-response-body

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 was debating doing that since the param for the document update is different, I think it needs to be separated between what we get back and what is sent for a document update

}

/**
Expand Down