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

Migrate to JAX-RS from raw servlet #82

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Migrate to JAX-RS from raw servlet #82

merged 1 commit into from
Oct 15, 2019

Conversation

ebyhr
Copy link
Contributor

@ebyhr ebyhr commented Sep 16, 2019

No description provided.

@ebyhr
Copy link
Contributor Author

ebyhr commented Sep 16, 2019

@wyukawa Could you enable Travis CI integration in this repository when you have time?

@ebyhr ebyhr mentioned this pull request Sep 28, 2019
38 tasks
build.gradle Outdated Show resolved Hide resolved
@@ -129,6 +130,7 @@ dependencies {
compile 'com.github.wyukawa.elasticsearch.unofficial.jdbc.driver:elasticsearch-jdbc-driver:0.0.9'
compile 'org.jsoup:jsoup:1.11.3'
compile 'mysql:mysql-connector-java:5.1.47'
compile 'com.owlike:genson:1.6'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with JSON library but is genson necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This library is needed when we set json key name explicitly.
eg @JsonProperty("formattedQuery")

@wyukawa
Copy link
Contributor

wyukawa commented Oct 9, 2019

enabled Travis CI integration in this repository like https://github.com/yanagishima/yanagishima/pull/106/checks?check_run_id=253262182

@ebyhr ebyhr changed the title Migrate to JAX-RS from raw servlet [WIP] Migrate to JAX-RS from raw servlet Oct 9, 2019
@ebyhr ebyhr changed the title [WIP] Migrate to JAX-RS from raw servlet Migrate to JAX-RS from raw servlet Oct 12, 2019
Copy link
Contributor

@wyukawa wyukawa left a comment

Choose a reason for hiding this comment

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

LGTM

@wyukawa wyukawa merged commit 643ec15 into yanagishima:master Oct 15, 2019
@wyukawa
Copy link
Contributor

wyukawa commented Oct 15, 2019

@ebyhr
I merged and deployed but it didn't work.
http://localhost:8080 is 404

It works after I commented out the followng line

servletContextHandler.addServlet(new ServletHolder(new ServletContainer(new YanagishimaResourceConfig(config))), "/*");

So I reverted in 5e2f0c2

Could you please check?

@ebyhr
Copy link
Contributor Author

ebyhr commented Oct 15, 2019

Found the root cause. The default servlet path was overwritten with the above code. Once we swap the order as below, the endpoint will return the expected result.

        servletContextHandler.addServlet(new ServletHolder(new ServletContainer(new YanagishimaResourceConfig(config))), "/*");
        servletContextHandler.addServlet(DefaultServlet.class, "/");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants