Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

Effector invoke API accepts form encoded data#75

Merged
asfgit merged 6 commits intoapache:masterfrom
sjcorbett:effector-http
Jul 18, 2014
Merged

Effector invoke API accepts form encoded data#75
asfgit merged 6 commits intoapache:masterfrom
sjcorbett:effector-http

Conversation

@sjcorbett
Copy link
Contributor

A bunch of uncontroversial commits and 85a2b0c which adds FormMapProvider to marshal an InputStream to a Map<String, Object>. It's used in the effector API to let the invoke method accept form-encoded data.

@sjcorbett sjcorbett changed the title Effectors accept form encoded data Effector invoke API accepts form encoded data Jul 16, 2014
@sjcorbett
Copy link
Contributor Author

I added FormMapProvider to what I thought were the relevant classes. Do I need to make any change to rest-api/web.xml? I considered adding the util package containing the class to the com.sun.jersey.config.property.packages section, but wasn't sure.

Sam Corbett added 5 commits July 16, 2014 19:19
* Adds FormMapProvider, which reads an InputStream into a
  Map<String, Object>, where the Object values are either a String, a
  List<String> or null.
* Uses this in EffectorApi to support form encoded data.
* Changes signature of EffectorApi.invoke parameters argument from
  Map<String, String> to Map<String, Object>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Preference for Map<String, ? extends Object> so can be more easily called. Not particularly important here as main usage is REST api.

However, I think it's possible to use the EffectorApi with some library goodness to provide a client java api that turns its calls into REST request/response. Do we have that out of interest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean using tools like Retrofit? I think it's probably a change worth making.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe not. Changing the signature to Map<String, ? extends Object> results in Jersey warning:

WARNING: The following warnings have been detected with resource and/or provider classes:
  WARNING: Parameter 5 of type java.util.Map<java.lang.String, ?> from public abstract javax.ws.rs.core.Response brooklyn.rest.api.EffectorApi.invoke(java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.util.Map<java.lang.String, ?>) is not resolvable to a concrete type

And the test added to ApplicationResourceTest fails too.

@aledsage
Copy link
Contributor

Looks really good. Only some very minor comments. Let me know when you've looked over those and think it's ready to merge.

@neykov
Copy link
Member

neykov commented Jul 18, 2014

+1 from me as well.

@asfgit asfgit merged commit 8319558 into apache:master Jul 18, 2014
asfgit pushed a commit that referenced this pull request Jul 18, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants