Skip to content
This repository has been archived by the owner on May 17, 2021. It is now read-only.

add optional headers functionality to http binding cache items in the main configuration file. #1371

Merged
merged 1 commit into from
Sep 12, 2014

Conversation

spali
Copy link
Contributor

@spali spali commented Aug 27, 2014

I know this is more a feature than a bug, but I would wish it on a stable code base, it can be used as soon as possible.
My use case is, that I need to send a login cookie in a HTTP request. Because I need several items with the same request, it makes sense to cache that, to be sure the box will won't explode due to many requests ;)

I made it with as less changes as possible, that's the reason why the parseHttpHeaders function now exists twice in the binding.
Let me know if I should change that and were you would like to have the public static parseHttpHeaders function in that case.

One side note:
I have seen that in the current code, Matcher is always used like this:

                    Matcher matcher = EXTRACT_CACHE_CONFIG_PATTERN.matcher(key);

                    if (!matcher.matches()) {
                        logger.error("given config key '"
                                + key
                                + "' does not follow the expected pattern '<id>.<url|updateInterval>'");
                        continue;
                    }   
                    matcher.reset();
                    matcher.find();

                    String cacheId = matcher.group(1);

but in my opinion the following would do the same:

                    Matcher matcher = EXTRACT_CACHE_CONFIG_PATTERN.matcher(key);

                    if (!matcher.matches()) {
                        logger.error("given config key '"
                                + key
                                + "' does not follow the expected pattern '<id>.<url|updateInterval>'");
                        continue;
                    }

                    String cacheId = matcher.group(1);

What me confused, was that I sometimes hat trouble with the first example, but didn't find a good reason why. In my opinion both should work, but the first has a bit overhead.
Maybe I can learn something new and get an explanation, because I'm not a full time java developer :)

Let me know if i should change anything and would be great if this pull request would find the way to master to (or do I need to pull it there too?)

A second side note:
If I find the time to extend this a bit more, I would also suggest to allow REGEX in the cache item, so the retrieved string could be stripped down to the minimum required in the cache to save memory. Because if you rely heavily on cache item from damn big ugly html sites, than this would consume a lot of memory, especially for embedded devices.

@buildhive
Copy link

openhab » openhab #1116 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@spali
Copy link
Contributor Author

spali commented Aug 28, 2014

build failed due two failed tests in org.openhab.core.library.types which is not touched in this PR

@teichsta teichsta added this to the 1.6.0 milestone Sep 12, 2014
@teichsta teichsta added the esh label Sep 12, 2014
teichsta added a commit that referenced this pull request Sep 12, 2014
add optional headers functionality to http binding cache items in the main configuration file.
@teichsta teichsta merged commit 80b1ef3 into openhab:1.5.1 Sep 12, 2014
@teichsta
Copy link
Member

Regarding your question about "matcher.reset()". You are probably right that the call to reset() is not necessary anymore. I thought it was necessary since "matches()" somehow consumes the pattern which is then not usable any further.

@spali
Copy link
Contributor Author

spali commented Sep 14, 2014

thanks for merging. Should create a PR for master branch or could you do it?

@kaikreuzer kaikreuzer removed the esh label Sep 14, 2014
@kaikreuzer
Copy link
Member

I have just cherry-picked your commit for the master branch - thanks for noticing that it was missing there!

hubermi pushed a commit to hubermi/openhab that referenced this pull request Jan 10, 2017
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants