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

Parse URL if bad URI #70947

Closed
wants to merge 3 commits into from
Closed

Conversation

legoguy1000
Copy link

  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Mar 27, 2021
@jrodewig jrodewig added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v7.13.0 v8.0.0 labels Mar 30, 2021
@legoguy1000
Copy link
Author

Is there a good way to test this change?? I'm looking at this page, https://github.com/elastic/elasticsearch/blob/master/TESTING.asciidoc, but I'm not really a java guy so IDK what the generic test command is.

@legoguy1000
Copy link
Author

This is what i get when I run gradle chack

alex@dev:~/elasticsearch$ gradle check
Starting a Gradle Daemon (subsequent builds will be faster)

FAILURE: Build failed with an exception.

* What went wrong:
Could not create service of type ScriptPluginFactory using BuildScopeServices.createScriptPluginFactory().
> Could not create service of type PluginResolutionStrategyInternal using BuildScopeServices.createPluginResolutionStrategy().

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 5s

@legoguy1000 legoguy1000 marked this pull request as ready for review March 31, 2021 16:44
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Mar 31, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (Team:Core/Features)

Copy link
Contributor

@danhermann danhermann left a comment

Choose a reason for hiding this comment

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

@legoguy1000, what are you trying to achieve with this change? Since URLs are a subset of URIs, I wouldn't expect there to be a case where a string was an invalid URI but a valid URL.

@legoguy1000
Copy link
Author

legoguy1000 commented Mar 31, 2021

@legoguy1000, what are you trying to achieve with this change? Since URLs are a subset of URIs, I wouldn't expect there to be a case where a string was an invalid URI but a valid URL.

The URI type fails when parsing a url with non URL encoded values like spaces in the path. The URL type does not.

@danhermann
Copy link
Contributor

This is what i get when I run gradle chack

What happens if you use the included gradle wrapper via ./gradlew check?

@legoguy1000
Copy link
Author

This is what i get when I run gradle chack

What happens if you use the included gradle wrapper via ./gradlew check?

alex@dev:~/elasticsearch$ ./gradlew check

FAILURE: Build failed with an exception.

* Where:
Settings file '/home/alex/elasticsearch/settings.gradle'

* What went wrong:
Could not compile settings file '/home/alex/elasticsearch/settings.gradle'.
> startup failed:
  General error during semantic analysis: Unsupported class file major version 60
  
  java.lang.IllegalArgumentException: Unsupported class file major version 60
        at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:196)
        at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:177)
        at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:163)
        at groovyjarjarasm.asm.ClassReader.<init>(ClassReader.java:284)
        at org.codehaus.groovy.ast.decompiled.AsmDecompiler.parseClass(AsmDecompiler.java:81)
        at org.codehaus.groovy.control.ClassNodeResolver.findDecompiled(ClassNodeResolver.java:251)
        at org.codehaus.groovy.control.ClassNodeResolver.tryAsLoaderClassOrScript(ClassNodeResolver.java:189)
        at org.codehaus.groovy.control.ClassNodeResolver.findClassNode(ClassNodeResolver.java:169)
        at org.codehaus.groovy.control.ClassNodeResolver.resolveName(ClassNodeResolver.java:125)
....

@legoguy1000
Copy link
Author

Like I said I'm not a Java guy so IDK if i've even installed all the dev dependences that i need

@danhermann
Copy link
Contributor

It looks like you're using JDK16 which does not appear to work well with our build system, yet. Try switching to JDK15 and doing a ./gradlew clean check.

@danhermann
Copy link
Contributor

@elasticmachine ok to test

@danhermann
Copy link
Contributor

@legoguy1000, if you're having trouble with the Java build framework on this one, I can pick it up since I now understand what the issue is. Let me know what you prefer.

@legoguy1000
Copy link
Author

legoguy1000 commented Apr 8, 2021

@legoguy1000, if you're having trouble with the Java build framework on this one, I can pick it up since I now understand what the issue is. Let me know what you prefer.

I haven't had a chance to do much with it lately. Feel free to push whatever changes you think are needed. I would like to know what i'm missing or what I should change/do for next time.

@danhermann
Copy link
Contributor

I haven't had a chance to do much with it lately. Feel free to push whatever changes you think are needed. I would like to know what i'm missing or what I should change/do for next time.

Cool, I'll get a fix in for this before the next patch release. If you want to open a thread about getting your local build working in our forums (https://discuss.elastic.co/), tag me (@danhermann) in it and I'll be happy to work through those with you there.

@danhermann
Copy link
Contributor

The resolution for this issue was merged in #71559.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants