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

CAMEL-13747: Added basic auth support to camel-solr #3034

Closed
wants to merge 1 commit into from
Closed

CAMEL-13747: Added basic auth support to camel-solr #3034

wants to merge 1 commit into from

Conversation

renjfk
Copy link
Contributor

@renjfk renjfk commented Jul 11, 2019

This is an attempt of implementing basic auth on solr component according to recommended way mentioned here. Concurrent client seems to be failing though for some reason when basic auth is enabled.

@renjfk
Copy link
Contributor Author

renjfk commented Jul 11, 2019

Ah well it was never implemented on ConcurrentUpdateSolrClient

@oscerd
Copy link
Contributor

oscerd commented Jul 11, 2019

Can you please raise also a issue here? https://issues.apache.org/jira/projects/CAMEL so we can track it better?

@renjfk renjfk changed the title Added basic auth support to camel-solr Added basic auth support to camel-solr (CAMEL-13747) Jul 11, 2019
@renjfk
Copy link
Contributor Author

renjfk commented Jul 11, 2019

Yes of course, just created.

@oscerd
Copy link
Contributor

oscerd commented Jul 11, 2019

And can you please open this against master branch and against camel-2.x? 2.24.x is already out and there is already 2.24.1. This is a new feature, so it should be part of camel-2.25.0. Thanks. You can close this PR and open another one against camel-2.x or closing this once you'll open the new one.

@renjfk renjfk changed the base branch from camel-2.24.x to camel-2.x July 11, 2019 15:15
@renjfk renjfk changed the base branch from camel-2.x to master July 11, 2019 15:16
@renjfk
Copy link
Contributor Author

renjfk commented Jul 11, 2019

Sure, changed this PR against master do you want me to create another PR against camel-2.x?

@oscerd
Copy link
Contributor

oscerd commented Jul 11, 2019

Yes. Please

@renjfk
Copy link
Contributor Author

renjfk commented Jul 11, 2019

Oki before that if you can review I have a small issue. I enabled basic auth for all tests so the feature can be tested along with all available methods but right now streaming tests are failing because of ConcurrentUpdateSolrClient does not implement it at all. Setting blockUnknown in security.json false would let it pass and other tests still wouldn't lose purpose because Solr only allows requests without Authorization header but still checks the ones with it. What do you think?

@oscerd
Copy link
Contributor

oscerd commented Jul 11, 2019

I think it makes sense to set blockUnknown to false for tests purpose

@renjfk renjfk changed the title Added basic auth support to camel-solr (CAMEL-13747) CAMEL-13747: Added basic auth support to camel-solr Jul 12, 2019
@renjfk
Copy link
Contributor Author

renjfk commented Jul 12, 2019

Alright changed it that way.

@renjfk
Copy link
Contributor Author

renjfk commented Jul 12, 2019

Another thing occurred to me. Currently we use this component on production and input body comes as Map and we are handling using custom type converter to SolrInputDocument. Not sure if that sounds a common use but if it does maybe it would make sense to extend component to handle Maps and/or Collection of Maps within the component. What do you think?

@oscerd
Copy link
Contributor

oscerd commented Jul 12, 2019

It could be part of another issue and another PR. Thanks

@renjfk
Copy link
Contributor Author

renjfk commented Jul 13, 2019

Alright then.

@oscerd
Copy link
Contributor

oscerd commented Jul 15, 2019

Thanks, merged on master

@oscerd oscerd closed this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants