New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #1712: Accounts API - dont return .json suffix for Uri resource #1715

Merged
merged 2 commits into from Jan 18, 2017

Conversation

Projects
None yet
4 participants
@abdulazizali77
Contributor

abdulazizali77 commented Jan 14, 2017

This is also related to #898
@otsakir @deruelle This PR might not be applicable if we change our minds

@otsakir otsakir added the Peer Review label Jan 14, 2017

@deruelle deruelle requested a review from otsakir Jan 14, 2017

@deruelle

This comment has been minimized.

Member

deruelle commented Jan 14, 2017

Thanks @abdulazizali77. @otsakir please review

@deruelle

This comment has been minimized.

Member

deruelle commented Jan 14, 2017

@abdulazizali77 we may change our mind as part of #898 review but at least the uri is consistent with the API call that was made.
#898 should solve the inconsistencies across the board but it may take a bit of time to fix them all so I'm fine with moving forward on consistency iteratively

@abdulazizali77 abdulazizali77 force-pushed the abdulazizali77:BUG1712 branch from 509dd5f to af3f8dc Jan 17, 2017

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

@otsakir please rereview
An alternative fix without modifying method signatures wouldve been to check against request.getCintentType() == 'application/json', bit kludgy but wouldnt have to pass an extra param, tell me what you think.

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 17, 2017

@abdulazizali77, i think it will work for now. It's better to put an exception to the rule in the upper layers (endpoint) like you did than hide it in the converter. Let me get on with the merge..

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 17, 2017

Hmmm, @abdulazizali77, the concept does not work. createFrom() is not involved when returning accounts. Only when creating new ones. The initial approach seems better after all.

In AccountConverter.java, i would overload prefix() so that it takes an additional content type parameter and use this overloated version from writeUri(final URI uri, final JsonObject object) which i know works for JSON content type. wdyt ?

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

@otsakir I agree with your idea but i would rather use MediaType like everywhere else

private String prefix(final Account account, final MediaType responseType) 

What do you think?
This would also mean that we wouldnt be using the persisted uri at all in returning the response! We will address that issue later lol i take that back it would still be used in xml response. However it still is a concern for consistency

I realize now that my prior implementation wouldve mapped a specific Account ID to either XML or JSON uri, when it shouldnt actually be exclusively either!

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 17, 2017

@otsakir I agree with your idea but i would rather use MediaType like everywhere else

ok @abdulazizali77, sounds great.

This would also mean that we wouldnt be using the persisted uri at all in returning the response!

Hmm, indeed. Wondering whether we should involve the data layer or not here. Btw, there has also been a suggestion in the past to get rid of content-type suffixes (.json etc.) and use the respective http headers instead. What is your opinion on that ?

I realize now that my prior implementation wouldve mapped a specific Account ID to either XML or JSON uri, when it shouldnt actually be exclusively either!

well, that's what PRs are for. A second pair of eyes never harmed anyone ;-)

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

well, that's what PRs are for. A second pair of eyes never harmed anyone ;-)

Ha, im sure everyone has had 'extreme programming' with three engineers and one manager behind them.... :D

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

Btw, there has also been a suggestion in the past to get rid of content-type suffixes (.json etc.) and use the respective http headers instead. What is your opinion on that ?

String/URI rewriting is a bad thing i feel, but i think we should support and standardize what we have right now for this version and add content type header based webservices in the next version on top of the hopefully deprecated suffix based one

@abdulazizali77 abdulazizali77 force-pushed the abdulazizali77:BUG1712 branch from af3f8dc to 9896841 Jan 17, 2017

Issue #1712: Accounts API - rewrite uri if JSON. add new writeUri int…
…erface taking Account. add new prefix method with additional MediaType param.

@abdulazizali77 abdulazizali77 force-pushed the abdulazizali77:BUG1712 branch from 9896841 to 1d203a6 Jan 17, 2017

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

@otsakir ready

@otsakir otsakir self-assigned this Jan 18, 2017

@gvagenas gvagenas merged commit dbab3eb into RestComm:master Jan 18, 2017

@gvagenas gvagenas removed the Peer Review label Jan 18, 2017

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Jan 18, 2017

Thanks @abdulazizali77. Thanks @otsakir for review. PR is merged

@otsakir otsakir added this to the 8.1.0 milestone Jan 18, 2017

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