dsmiley commented on code in PR #4272: URL: https://github.com/apache/solr/pull/4272#discussion_r3090847555
########## changelog/unreleased/SOLR-18056-urlScheme-csp.yml: ########## @@ -1,8 +1,15 @@ # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled". +title: Improved url scheme detection in client API and core components Review Comment: IMO this has become much more vague to a reader of the changelog who is anyone other than you and me. The "title" label is misleading -- add more sentences. ########## changelog/unreleased/SOLR-18056-urlScheme-csp.yml: ########## @@ -1,8 +1,15 @@ # See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc -title: Improved CloudSolrClient's urlScheme detection by using the scheme of provided Solr URLs, or looking at "solr.ssl.enabled". +title: Improved url scheme detection in client API and core components type: changed authors: - name: Vishnu Priya Chandra Sekar links: - name: SOLR-18056 url: https://issues.apache.org/jira/browse/SOLR-18056 + - name: SOLR-18055 + url: https://issues.apache.org/jira/browse/SOLR-18055 +important_notes: Review Comment: This is the first I've seen of this... and you are the first to attempt to use it :-) Our only references to it thus far don't even truly say what it's for. [Documentation.](https://logchange.dev/tools/logchange/faq/?h=important_notes#q-how-do-i-handle-breaking-changes) Seems for "breaking changes" or presumably anything befitting of that label. What you added doesn't qualify IMO. Well the "deprecated" part does... albeit I think I said it's premature to deprecate that. ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java: ########## @@ -433,6 +432,31 @@ private String buildUrl(String url) { return url; } + /** + * Get url scheme of host + * + * <p>Examples: + * + * <ul> + * <li>Returns {@code "https"} when SSL is enabled via configuration. + * <li>Returns {@code null} when ssl is disabled or no explicit configuration is provided in + * solr.xml. + * </ul> + * + * @return http or https or null + */ + private String getUrlScheme(NamedList<?> args, StringBuilder sb) { Review Comment: maybe prefix with 'init' not 'get' to emphasize this is for initialization and not any method in this class that might want it. On further reflection, there's `getParameter` called by init() a bunch of times. This method here was merely extracted from init(). You could put it back. Or call it `getParameterUrlScheme` ########## solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java: ########## @@ -433,6 +432,31 @@ private String buildUrl(String url) { return url; } + /** + * Get url scheme of host + * + * <p>Examples: + * + * <ul> + * <li>Returns {@code "https"} when SSL is enabled via configuration. + * <li>Returns {@code null} when ssl is disabled or no explicit configuration is provided in + * solr.xml. + * </ul> Review Comment: sorry, this is *way* too much documentation for a private method -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
