You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by "Cassie Doll (JIRA)" <ji...@apache.org> on 2008/09/24 19:39:44 UTC
[jira] Commented: (SHINDIG-601) Input format detection
[ https://issues.apache.org/jira/browse/SHINDIG-601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12634213#action_12634213 ]
Cassie Doll commented on SHINDIG-601:
-------------------------------------
A couple of comments on this patch -
- watch your spacing. if statements should look like this "if (contentType != null) {" space before open paren, no space after open paren. space between close paren and bracket
- make sure you aren't using any tab chars
- unfortunately, your patch just got out of sync because of another patch which added a BeanAtomConverter... so now atom and xml are separate
- this method has a test in DataServiceServletTest. make sure you are running it with mvn test and add new test cases to it.
- the "CONTENT_TYPE" string and all other strings should be pulled into constants at the top of the file (next to FORMAT_PARAM and the others)
And one last overall comment. I don't think Chris's original description on this bug was clear enough. Once we add this content type detection code in we really need to use two separate converters. One to parse the incoming post data and one to format our output. So then if you posted xml with format=json things would work as they should. (Seems silly, but it shouldn't be hard to do it right I suppose)
This shouldn't be too hard to change in the DataServiceServlet - are you up for it?
Sorry for the large amount of comments and thanks for your help!
> Input format detection
> ----------------------
>
> Key: SHINDIG-601
> URL: https://issues.apache.org/jira/browse/SHINDIG-601
> Project: Shindig
> Issue Type: Bug
> Components: RESTful API (Java), RESTful API (PHP)
> Reporter: Chris Chabot
> Priority: Blocker
> Attachments: fix-601-bug.patch, inputContentType.patch
>
>
> Currently PHP uses the content_type header to detect the input format. On the other hand Java uses the format query param (?format=foo) for the input format selection.
> Most logical solution seems to be that we both use :
> if ( content type is set)
> use content_type
> else if (format query param is set)
> use query param
> else
> use json
> I *think* that will be what developers would expect :)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (SHINDIG-601) Input format detection
Posted by Cassie <do...@google.com>.
On Wed, Sep 24, 2008 at 10:50 AM, Rajdeep Dua <ra...@google.com> wrote:
> Does this mean application/xml, and application/atom+xml are now supported?
> xml for BeanXmlConverter and atom+xml for atomConverter
i think this is correct.
> Regarding the tests,
> I will have the create an instance for HttpServletRequest -- what is the
> easiest way of doing this? Do I use jetty's implementation class of this
> interface to create a Mock
In DataServiceServlet line 194 you will see the test that already exists.
The current test method does not access the servletRequest yet but there are
a bunch of other examples in that class which do and they use easy mock. So
you can just get the req defined on line 49 and follow the format of the
other tests.
>
> Thanks
> Rajdeep
>
>
> On Wed, Sep 24, 2008 at 11:09 PM, Cassie Doll (JIRA) <jira@apache.org
> >wrote:
>
> >
> > [
> >
> https://issues.apache.org/jira/browse/SHINDIG-601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12634213#action_12634213
> ]
> >
> > Cassie Doll commented on SHINDIG-601:
> > -------------------------------------
> >
> > A couple of comments on this patch -
> >
> > - watch your spacing. if statements should look like this "if
> (contentType
> > != null) {" space before open paren, no space after open paren. space
> > between close paren and bracket
> > - make sure you aren't using any tab chars
> > - unfortunately, your patch just got out of sync because of another patch
> > which added a BeanAtomConverter... so now atom and xml are separate
> > - this method has a test in DataServiceServletTest. make sure you are
> > running it with mvn test and add new test cases to it.
> > - the "CONTENT_TYPE" string and all other strings should be pulled into
> > constants at the top of the file (next to FORMAT_PARAM and the others)
> >
> > And one last overall comment. I don't think Chris's original description
> on
> > this bug was clear enough. Once we add this content type detection code
> in
> > we really need to use two separate converters. One to parse the incoming
> > post data and one to format our output. So then if you posted xml with
> > format=json things would work as they should. (Seems silly, but it
> shouldn't
> > be hard to do it right I suppose)
> >
> > This shouldn't be too hard to change in the DataServiceServlet - are you
> up
> > for it?
> >
> > Sorry for the large amount of comments and thanks for your help!
> >
> > > Input format detection
> > > ----------------------
> > >
> > > Key: SHINDIG-601
> > > URL: https://issues.apache.org/jira/browse/SHINDIG-601
> > > Project: Shindig
> > > Issue Type: Bug
> > > Components: RESTful API (Java), RESTful API (PHP)
> > > Reporter: Chris Chabot
> > > Priority: Blocker
> > > Attachments: fix-601-bug.patch, inputContentType.patch
> > >
> > >
> > > Currently PHP uses the content_type header to detect the input format.
> On
> > the other hand Java uses the format query param (?format=foo) for the
> input
> > format selection.
> > > Most logical solution seems to be that we both use :
> > > if ( content type is set)
> > > use content_type
> > > else if (format query param is set)
> > > use query param
> > > else
> > > use json
> > > I *think* that will be what developers would expect :)
> >
> > --
> > This message is automatically generated by JIRA.
> > -
> > You can reply to this email to add a comment to the issue online.
> >
> >
>
Re: [jira] Commented: (SHINDIG-601) Input format detection
Posted by Rajdeep Dua <ra...@google.com>.
Does this mean application/xml, and application/atom+xml are now supported?
xml for BeanXmlConverter and atom+xml for atomConverterRegarding the tests,
I will have the create an instance for HttpServletRequest -- what is the
easiest way of doing this? Do I use jetty's implementation class of this
interface to create a Mock
Thanks
Rajdeep
On Wed, Sep 24, 2008 at 11:09 PM, Cassie Doll (JIRA) <ji...@apache.org>wrote:
>
> [
> https://issues.apache.org/jira/browse/SHINDIG-601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12634213#action_12634213]
>
> Cassie Doll commented on SHINDIG-601:
> -------------------------------------
>
> A couple of comments on this patch -
>
> - watch your spacing. if statements should look like this "if (contentType
> != null) {" space before open paren, no space after open paren. space
> between close paren and bracket
> - make sure you aren't using any tab chars
> - unfortunately, your patch just got out of sync because of another patch
> which added a BeanAtomConverter... so now atom and xml are separate
> - this method has a test in DataServiceServletTest. make sure you are
> running it with mvn test and add new test cases to it.
> - the "CONTENT_TYPE" string and all other strings should be pulled into
> constants at the top of the file (next to FORMAT_PARAM and the others)
>
> And one last overall comment. I don't think Chris's original description on
> this bug was clear enough. Once we add this content type detection code in
> we really need to use two separate converters. One to parse the incoming
> post data and one to format our output. So then if you posted xml with
> format=json things would work as they should. (Seems silly, but it shouldn't
> be hard to do it right I suppose)
>
> This shouldn't be too hard to change in the DataServiceServlet - are you up
> for it?
>
> Sorry for the large amount of comments and thanks for your help!
>
> > Input format detection
> > ----------------------
> >
> > Key: SHINDIG-601
> > URL: https://issues.apache.org/jira/browse/SHINDIG-601
> > Project: Shindig
> > Issue Type: Bug
> > Components: RESTful API (Java), RESTful API (PHP)
> > Reporter: Chris Chabot
> > Priority: Blocker
> > Attachments: fix-601-bug.patch, inputContentType.patch
> >
> >
> > Currently PHP uses the content_type header to detect the input format. On
> the other hand Java uses the format query param (?format=foo) for the input
> format selection.
> > Most logical solution seems to be that we both use :
> > if ( content type is set)
> > use content_type
> > else if (format query param is set)
> > use query param
> > else
> > use json
> > I *think* that will be what developers would expect :)
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>