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

Allow ip_range to accept CIDR notation #27192

Merged
merged 2 commits into from
Nov 3, 2017

Conversation

original-brownbear
Copy link
Member

My attempt at #26260 :

  • Used the same approach as in org.elasticsearch.search.aggregations.bucket.range.IpRangeAggregationBuilder.Range#Range(java.lang.String, java.lang.String) for the parsing (with some minor simplifications that could be made here by fixing the include/exclude behaviour of the boundaries)
  • added a new test class IpRangeFieldMapperTests since there were only (at least as far as I could find) generic test classes that covered all Range types in a loop that applied here

@talevy talevy added :Search Foundations/Mapping Index mappings, including merging and defining field types >enhancement labels Oct 31, 2017
@talevy talevy requested a review from nknize October 31, 2017 18:47
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Change looks good, I left a comment about the test.

ParsedDocument doc =
mapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder()
.startObject()
.field("field", "192.168.0.0/16")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test something that is not byte-aligned, like /15 or /17? We used to have bugs in those cases.

@dakrone dakrone self-assigned this Nov 3, 2017
@original-brownbear
Copy link
Member Author

@jpountz done, added the non-aligned test cases as well :)

@original-brownbear
Copy link
Member Author

Same as in the other PR, failed BWC => rebased against master to fix.

@original-brownbear
Copy link
Member Author

@dakrone this one's green after rebase now as well :)

@dakrone dakrone merged commit 3deba0e into elastic:master Nov 3, 2017
dakrone pushed a commit that referenced this pull request Nov 3, 2017
*  #26260 Allow ip_range to accept CIDR notation

*  #26260 added non-byte-alligned cidr test cases
@dakrone dakrone mentioned this pull request Nov 3, 2017
@original-brownbear original-brownbear deleted the 26260 branch November 4, 2017 07:54
martijnvg added a commit that referenced this pull request Nov 4, 2017
* es/master:
  Fix snapshot getting stuck in INIT state (#27214)
  Add an example of dynamic field names (#27255)
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix InternalStatsTests
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
martijnvg added a commit that referenced this pull request Nov 4, 2017
* 6.x:
  Add an example of dynamic field names (#27255)
  fixed checkstyle violation
  #26260 Allow ip_range to accept CIDR notation (#27192)
  #27189 Fixed rounding of bounds in scaled float comparison (#27207)
  [TEST] Fix failing exists query test
  test: Do not run old parent/child tests against a cluster with minimum version greater than 6.0.0
  Add support for Gradle 4.3 (#27249)
  Fixes QueryStringQueryBuilderTests
  build: Fix setting the incorrect bwc version in mixed cluster qa module
  fix compil after backport
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery BWC
  Adjust assertions for sequence numbers BWC tests
  Do not create directories if repository is readonly (#26909)
  [Test] Fix QueryStringQueryBuilderTests.testExistsFieldQuery
  Uses norms for exists query if enabled (#27237)
  Reinstate recommendation for ≥ 3 master-eligible nodes. (#27204)
@clintongormley clintongormley changed the title #26260 Allow ip_range to accept CIDR notation Allow ip_range to accept CIDR notation Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search Foundations/Mapping Index mappings, including merging and defining field types v6.1.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants