fix(_common): rename ShardFailure fields to match server response - #1194
fix(_common): rename ShardFailure fields to match server response#1194gingeekrishna wants to merge 3 commits into
Conversation
OpenSearch may omit the shard field in ShardFailure responses for certain failure types (e.g. index-level failures where no specific shard is implicated). The field was incorrectly listed as required, causing MissingRequiredPropertyException in the Java client when deserializing responses from opensearch-project/opensearch-java#1799. Remove shard from the required list; it remains defined as an integer property so it is still serialized/deserialized when present. Signed-off-by: Radha Krishnan P <gingeekrishna@gmail.com>
| required: | ||
| - primary | ||
| - reason | ||
| - shard |
There was a problem hiding this comment.
I looked at the server code, sorry I don't think removing shard from required is the right fix here.
ShardFailure schema is used inside ShardInfo.failures (line 315), which corresponds to ReplicationResponse.ShardInfo.Failure on the server side. that class always writes the shard field, it never omits it:
builder.field(_SHARD, shardId.id());
builder.field(_INDEX, shardId.getIndexName());
builder.field(_NODE, nodeId);
real problem is a field name mismatch. server writes _shard, _index, _node (with underscores), but this spec defines them as shard, index, node (without underscores). so the Java client looks for shard in the JSON, doesn't find it because the actual key is _shard, and throws MissingRequiredPropertyException.
making shard optional just hides this, client will silently get null instead of the actual shard ID. fix should be correcting the property names to match what the server actually returns.
Can you check the actual JSON response from the server that triggered the original issue? I'd expect the fields are there but named _shard, _index, _node.
There was a problem hiding this comment.
Good catch, thanks for checking the server code. You're right, removing shard from required just masked the real bug. Fixed in 646c39d: renamed index/node/shard to _index/_node/_shard to match what ReplicationResponse.ShardInfo.Failure actually writes, and restored _shard as required since the server always includes it. Also updated the PR title/description accordingly.
ReplicationResponse.ShardInfo.Failure on the server always writes the shard field, so making it optional just masked a MissingRequiredPropertyException instead of fixing it. The real issue is a name mismatch: the server writes _index, _shard, _node (underscore-prefixed) while this schema defined index, shard, node without the prefix, so clients looked up the wrong key and got null/missing errors. Rename the properties to _index, _shard, _node to match what the server actually serializes, and restore shard (as _shard) as required since the server always includes it. Addresses review comment from @iprithv on opensearch-project#1194. Signed-off-by: Radha Krishnan P <gingeekrishna@gmail.com>
gingeekrishna
changed the title
Description
Rename the
ShardFailureschema properties inspec/schemas/_common.yamlfromindex/node/shardto_index/_node/_shardto match whatReplicationResponse.ShardInfo.Failureon the OpenSearch server actually serializes (underscore-prefixed keys for_index,_shard,_node; no prefix forreason,status,primary).An earlier version of this PR instead removed
shardfromrequired, on the assumption that the server sometimes omits it. As pointed out in review, the server always writes the field — the real problem was the name mismatch, so clients were looking upshardand finding nothing under that key while the actual value sat under_shard. This restores_shardasrequiredand renames all three affected properties.Related Issues
Reported via opensearch-java#1799 and the corresponding fix opensearch-java#2037.
Checklist
_shardrestored asrequired, matching server behavior;_index/_node/_shardnow match the server's actual JSON keysSigned-off-by: Radha Krishnan P gingeekrishna@gmail.com