Some issues with custom VarExpander #26

Closed
drdamour opened this Issue Apr 24, 2015 · 10 comments

Projects

None yet

2 participants

@drdamour

Using this library on android, which doesn't have java.beans.Introspector so the default var expander doesn't work.

I have a pretty simple use case of a template like /path{?multivar*} for which i want to set("multivar", new String[]{"one", "two"}) thus expanding to /path?multivar=one&multivar=two

It throws becuase the DefaultVarExpander can't get Introspector. It seems like a re-org of some of the order of things to determine if it's a "simple" array would get me away from this problem, but I also saw that I can specify my won var expander so i went there.

small asside on that, the java docs say there are two ways to register a custom car expander but only explain one way by wrapping the variable. The readme only mentions one way, maybe a planned feature that never made it was the 2nd way?

Anyways i built a simple expander that to an array of strings and spit it out the getValues method as a collection, but instead got my param passed as the toString() of the object. After stepping through the code i found that if you use a custom var expander only the getPairs() method is considered. So there wasn't a natural way to do what i wanted. I ended up hacking something that fell through the right if conditions enough to work, but clearly this is very much internal implementation dependent:

class Selections extends ArrayList<String> implements VarExploder {

        public Selections(String... selections){
            this.addAll(Arrays.asList(selections));
        }

        @Override
        public Map<String, Object> getNameValuePairs() throws VariableExpansionException {
            return null;
        }

        @Override
        public Collection<Object> getValues() throws VariableExpansionException {
            return null;
        }
    }

it works and all, but I'm guessing this isn't the intent.

so tl;dr:

  • figure a way to support arrays of simple values without a need for java.bean.Introspector
  • doc the dependency on java.bean.Introspector for android devs
  • fix docs about the 2nd way to register an Introspector
  • maybe fallback to a var expander built on openbeans https://code.google.com/p/openbeans/
  • add a way to allow a custom var expander to work with getValues()

maybe the last one is possible and i just couldn't figure it out.

anyways, have been using your lib quite a bit, thanks!

@damnhandy
Owner

Thanks for these @drdamour ! The docs on the VarExploder is definitely unfinished business. I had an idea of being able to load custom implementations via classes marked with an annotation, but the code was never implemented. As for java.bean.Introspector, I have to think about this more. Right now, it saves a lot of code. I never considered Android use cases until now and totally wasn't aware that it did not include this package.

I see a few ways around this:

  • Dump java.bean.Introspector completely in favor of custom code that does the same thing
  • Use something like OpenBeans as you suggested
  • Create a separate, Android-specific module that is separate from the main library

Any preferences?

@drdamour

i was suggesting dumping Introspector, but for my use case (array of values) change the code path so it's not needed, it seemed like the order of operations uses Introspector too soon.

separate module is ok...but i'm building a cross platform lib that depends on damnhand uri, so I'd have to mark that as provided and deal with all the questions, unless i make use of some maven thing that deals with platforms (not even sure that exists). So i'm not a bit fan of that exactly. If the module overrides built in implementation some how, that'd be cooler. If there was another needed divergence then i probably would make sense to have a spec/interface module and implementation backer.

Depending on openbeans outside of android sucks just cause it's another dependency that JVM people won't want.

If you had that second way to do custom expanders then it just becomes a documentation thing.

@damnhandy
Owner

You're right, I think dumping Introspector for something custom probably makes the most sense. I think a light-weight, home grown, reflection-based approach would be ideal. I'd like to keep additional dependencies out of the project if possible. I'll try and give that a shot over the week, but the day job has me pretty tied up at the moment. Contributions are welcome though :)

@damnhandy damnhandy added this to the 2.1 milestone Sep 13, 2015
@damnhandy damnhandy self-assigned this Sep 13, 2015
@drdamour
drdamour commented Nov 4, 2015

ran across this again, this time i was trying to pass in a Map. I had time so i started a fork, but then i got really confused with what you're trying to do in there with the factory.

A lot of this could be simply allieviated by moving the call to the VarExploderFactory to AFTER the default cases are handled.

Right now the code is doing this block https://github.com/damnhandy/Handy-URI-Templates/blob/master/src/main/java/com/damnhandy/uri/template/UriTemplate.java#L652 (which uses the factory and thus the problematic DefaultVarExploder) FIRST and then hits these if blocks https://github.com/damnhandy/Handy-URI-Templates/blob/master/src/main/java/com/damnhandy/uri/template/UriTemplate.java#L676 which don't care if the value is an VarExploder or not, just if they're the special cases that are handled.

Switching the order would avoid the VarExploder logic all together for most common cases of Collection or Map.

But i also wondered if you even want to deep those default cases. Shouldn't those just be handled by the DefaultVarExploder. Or do you want a VarExploder for each of those types, and have the factory match them up. IE a MapVarExploder a CollectionVarExploder. And then URI template would have no special cases of values...just VarExploders

Not sure which direction you were heading. If you offer some guidance i can probably submit a PR.

@drdamour
drdamour commented Nov 4, 2015

also looks ike jackson ran into the same thing FasterXML/jackson-jr#11

@damnhandy
Owner

Interesting find! Since this library has a dependency on Jackson already, it should be possible to piggy back on Jackson.

@damnhandy
Owner

I ended up rolling my own, all tests are passing and I've added this to the 2.1.0-SNAPSHOT in Maven Central

@damnhandy damnhandy closed this Nov 8, 2015
@drdamour
drdamour commented Nov 8, 2015

great. is 2.1 ready for a release?

@damnhandy
Owner

Just released it. Just waiting for Sonatype to do it's thing so that it gets into Maven Central.

@drdamour
drdamour commented Nov 9, 2015

very nice! thanks for putting some effort behind this. I'll verify it works monday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment