5.5 #2807

Mpdreamz · 2017-07-14T13:19:39Z

This updates the json spec to 5.5 and generates the code from that. No big major changes to existing API's.

The machine learning json spec is included but we do not generate code from that folder just yet, this will follow in a 5.5.x release.

XPack security has a new role_mapping API to map roles to users, and a token based access API. Support for these have been added with tests.

The matrix_stats, sig_terms and sampler_agg have additional properties as per 5.5 which tripped up our aggregation parser.

GetAlias now return a 404 if you submit a query for e.g a,b,c and only alias a was found as per: elastic/elasticsearch#25043

… new features

…aliases suplied are not found

Removed in REST API spec elastic/elasticsearch#25234 - patched in to maintain binary compatibility

russcam

Beast of a PR! Some questions around bitwise operators and semantics for valid Get and Delete responses

russcam · 2017-07-17T00:44:43Z

src/Tests/Framework/ManagedElasticsearch/Clusters/XPackCluster.cs

@@ -11,9 +11,9 @@ public class XPackCluster : ClusterBase
 	{
 		protected string[] XPackSettings => TestClient.VersionUnderTestSatisfiedBy(">=5.5.0")
 			? new[] {"xpack.security.authc.token.enabled=true"}
-			: Array.Empty<string>();


russcam · 2017-07-17T00:56:05Z

src/Nest/CommonAbstractions/SerializationBehavior/ElasticContractResolver.cs

@@ -38,10 +38,17 @@ protected override JsonContract CreateContract(Type objectType)
 			// cache contracts per connection settings
 			return this.ConnectionSettings.Inferrer.Contracts.GetOrAdd(objectType, o =>
 			{
+				var n = o.GetType().FullName;


superfluous - not used

russcam · 2017-07-17T00:56:16Z

src/Nest/CommonAbstractions/SerializationBehavior/ElasticContractResolver.cs

 				var contract = base.CreateContract(o);

 				if ((typeof(IDictionary).IsAssignableFrom(o) || o.IsGenericDictionary()) && !typeof(IIsADictionary).IsAssignableFrom(o))
 				{
+					if (contract.Converter != null)


russcam · 2017-07-17T01:00:25Z

src/Nest/Indices/AliasManagement/GetAlias/GetAliasResponse.cs

 	}

 	public class GetAliasResponse : ResponseBase, IGetAliasResponse
 	{
 		public IReadOnlyDictionary<string, IReadOnlyList<AliasDefinition>> Indices { get; internal set; } = EmptyReadOnly<string, IReadOnlyList<AliasDefinition>>.Dictionary;
+
+
+		public override bool IsValid => this.Indices.Count > 0 ? true : false;


can be shortened to

this.Indices.Count > 0;

russcam · 2017-07-17T02:30:45Z

src/CodeGeneration/ApiGenerator/RestSpecification/Core/indices.refresh.json

@@ -25,14 +25,6 @@
            "options" : ["open","closed","none","all"],
            "default" : "open",
            "description" : "Whether to expand wildcard expression to concrete indices that are open, closed or both."
-        },


This looks like a breaking API change which is going to break binary compatibility for Elasticsearch.Net.

Reading elastic/elasticsearch#25234, it looks like these should have been removed from the rest api specs for 5.x, but given these are methods on the client, I think they should still exist as Noops in 5.x to not break binary compatibility.

Fixed: 7c9b72a

russcam · 2017-07-17T04:12:01Z

src/Tests/ClientConcepts/Certificates/SslAndKpiXPackCluster.cs

-				return this._desiredPort.Value;
-			}
-		}
-


russcam · 2017-07-17T05:06:08Z

src/Elasticsearch.Net/Domain/RequestParameters/RequestParameters.Generated.cs

+	///https://www.elastic.co/guide/en/x-pack/master/security-api-role-mapping.html#security-api-delete-role-mapping
+	///</pre>
+	///</summary>
+	public class DeleteRoleMappingRequestParameters : FluentRequestParameters<DeleteRoleMappingRequestParameters> 


I think this should adhere to allowing a 404 status code response, similar to other Delete operations

Good catch!

russcam · 2017-07-17T05:28:48Z

src/Elasticsearch.Net/Domain/RequestParameters/RequestParameters.Generated.cs

+	///https://www.elastic.co/guide/en/x-pack/master/security-api-role-mapping.html#security-api-get-role-mapping
+	///</pre>
+	///</summary>
+	public class GetRoleMappingRequestParameters : FluentRequestParameters<GetRoleMappingRequestParameters> 


I think this should also be consistent with other Get* responses in Elasticsearch in returning a { "found": false } body along with 404 response. Speaking with @tvernum, this behaviour is consistent across Get* responses in X-Pack Security, but going to open up an issue to collectively discuss these.

russcam · 2017-07-17T05:49:21Z

src/Tests/XPack/Security/RoleMapping/PutRoleMapping/PutRoleMappingApiTests.cs

+			Rules =
+				(new DistinguishedNameRule(_dn) | new UsernameRule(_username) | new RealmRule(_realm))
+				& new MetadataRule(_metadata.Item1, _metadata.Item2)
+				& !new GroupsRule(_groups)


does it make sense to use logical operators here as opposed to bitwise, similarly to how they are used within queries? For example,

(new DistinguishedNameRule(_dn) || new UsernameRule(_username) || new RealmRule(_realm)) && new MetadataRule(_metadata.Item1, _metadata.Item2) && !new GroupsRule(_groups)

That sadly won't be possible since we need to return a different class when they are combined.

Could we have implementations of AnyFieldRule, AllFieldRule and ExceptFieldRule that derive from FieldRuleBase and implicitly convert to the associated RoleMappingRuleBase implementations, respectively? Would that work?

I think there's potential for confusion with using bitwise operators, since we use binary operators for combining queries, aggregations and watcher actions.

Implicit conversions and operator overloading are not friends :)

I hear you on the deviation from other areas though but as this is icing and not the only way to construct this (ois/fluent are also there). I am leaning more on the rtfm side of things here tbh.

We could call it out in the docs stronger though.

I think people might eat more of the icing than cake 🤣

FieldRuleCombinator a la ActionCombinator?

elasticsearch-net/src/Nest/XPack/Watcher/Action/ActionBase.cs

Lines 51 to 73 in 1de5538

internal class ActionCombinator : ActionBase, IAction

{

internal List<ActionBase> Actions { get; } = new List<ActionBase>();

public ActionCombinator(ActionBase left, ActionBase right) : base(null)

{

this.AddAction(left);

this.AddAction(right);

}

private void AddAction(ActionBase agg)

{

if (agg == null) return;

var combinator = agg as ActionCombinator;

if ((combinator?.Actions.HasAny()).GetValueOrDefault(false))

{

this.Actions.AddRange(combinator.Actions);

}

else this.Actions.Add(agg);

}

public override ActionType ActionType => (ActionType)10;

}

.

ActionBase overloaded operators return ActionBase so binary operators work. FieldRuleBase needs to return RoleMappingRuleBase so it can't.

russcam · 2017-07-17T05:50:13Z

src/Tests/XPack/Security/RoleMapping/RoleMappingRules.doc.cs

+	 * NEST exposes this role rules DSL in a strongy typed fashion
+	 *
+	 */
+	public class RoleMappingRulesTests : SerializationTestBase


X-Pack docs 😃

Mpdreamz · 2017-07-17T13:15:48Z

src/CodeGeneration/ApiGenerator/Overrides/Descriptors/RefreshDescriptorOverrides.cs

@@ -0,0 +1,23 @@
+using ApiGenerator.Domain;
+
+namespace ApiGenerator.Overrides.Descriptors


This can also be achieved by adding an *.obsolete.json companion file to the rest spec itself see:

https://github.com/elastic/elasticsearch-net/blob/100cf7c3f81553456fb039ca7a499a97c7990028/src/CodeGeneration/ApiGenerator/RestSpecification/Core/cat.thread_pool.obsolete.json

Can we move the *.obsolete.json and *.patch.json files to a different directory to the downloaded specs? I think it'll make it easier to manage

* Code generation of elasticsearch's 5.5 branch * Update existing XPack API spec files * Add ml xpack api spec and new security endpoints * Add CRUD support for the new role mapping security API's * Add support for security token API's * fix failing codestandards tests after implementing xpack security 5.5 new features * fix overal bg_count now gets returned for sig_terms * fix stats matrix now also returns overal doc_count * fix change in GetAlias behavior, since 5.5 now returns 404 when some aliases suplied are not found * Sampler agg test for bg count should only be asserted 5.5.0 and up * Array.Empty not available for .NET 4.5 * Add force + operation_threading to RefreshRequest Removed in REST API spec elastic/elasticsearch#25234 - patched in to maintain binary compatibility * Set tests.default.yaml ES version to 5.5.0 * Update XML comments for LazyDocument * Minor language tweaks * Fix spelling * update SkipVersion for PutRoleMapping to <5.5.0 (cherry picked from commit 5617675)

Mpdreamz and others added 14 commits July 14, 2017 14:47

Code generation of elasticsearch's 5.5 branch

6fa7f6b

Update existing XPack API spec files

4e813f5

Add ml xpack api spec and new security endpoints

87ae230

Add CRUD support for the new role mapping security API's

fa50dac

Add support for security token API's

6fb0c10

fix failing codestandards tests after implementing xpack security 5.5…

98bf770

… new features

fix overal bg_count now gets returned for sig_terms

adafd85

fix stats matrix now also returns overal doc_count

173aeb6

fix change in GetAlias behavior, since 5.5 now returns 404 when some …

0c2f0ae

…aliases suplied are not found

Sampler agg test for bg count should only be asserted 5.5.0 and up

c173c53

Array.Empty not available for .NET 4.5

81800de

Add force + operation_threading to RefreshRequest

7c9b72a

Removed in REST API spec elastic/elasticsearch#25234 - patched in to maintain binary compatibility

Set tests.default.yaml ES version to 5.5.0

6f6faea

Update XML comments for LazyDocument

0b4b765

russcam suggested changes Jul 17, 2017

View reviewed changes

codebrain added 2 commits July 17, 2017 16:24

Minor language tweaks

6d3b017

Fix spelling

100cf7c

Mpdreamz commented Jul 17, 2017

View reviewed changes

russcam approved these changes Jul 21, 2017

View reviewed changes

codebrain added the v5.4.1 label Jul 21, 2017

update SkipVersion for PutRoleMapping to <5.5.0

62079b9

russcam merged commit 5617675 into 5.x Jul 21, 2017

elasticmachine mentioned this pull request Apr 25, 2018

Response consistency of Get* and Delete* responses elastic/elasticsearch#29783

Open

	internal class ActionCombinator : ActionBase, IAction
	{
	internal List<ActionBase> Actions { get; } = new List<ActionBase>();

	public ActionCombinator(ActionBase left, ActionBase right) : base(null)
	{
	this.AddAction(left);
	this.AddAction(right);
	}

	private void AddAction(ActionBase agg)
	{
	if (agg == null) return;
	var combinator = agg as ActionCombinator;
	if ((combinator?.Actions.HasAny()).GetValueOrDefault(false))
	{
	this.Actions.AddRange(combinator.Actions);
	}
	else this.Actions.Add(agg);
	}

	public override ActionType ActionType => (ActionType)10;
	}

		@@ -0,0 +1,23 @@
		using ApiGenerator.Domain;

		namespace ApiGenerator.Overrides.Descriptors

5.5 #2807

5.5 #2807

Uh oh!

Conversation

Mpdreamz commented Jul 14, 2017

Uh oh!

russcam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!