pullrequest creation API

[Mads Kiilerich]

On 03/12/2015 01:40 PM, Thomas De Schampheleire wrote:
>>> At first sight, using the api_access=True method to create a callable
>>> web view is the easiest, since you could take in the request and pass
>>> it to the existing PullrequestsController:create() method. However,
>>> this existing create method is decorated with @NotAnonymous, causing
>>> it to be unavailable for API calls.
>>>
>>> The alternative is to create an API method in controllers/api/api.py,
>>> but it's unclear to me how to hook this into the
>>> PullrequestsController.
>>
>> I think that is because we have too much logic in the controller. I think
>> that in some MVC universes, the controller should only deal with the view
>> specific code (html page and user interaction). The logic we want to expose
>> for general purposes should thus not be view specific but should be a part
>> of the model.
> The way I understand the MVC concept:
> - the model is an encapsulation of the database
> - the view is the representation towards the user
> - the controller is the engine between both, taking data from the
> model and presenting it to the user, or taking input from the user and
> putting it in the model.
>
> When representing information about a pullrequest, as pull request #47
> is doing, one could argue that it could be retrieved from the model.
> But when trying to create a new pullrequest, as I am trying to
> accomplish, the model is not the right place I think. The parsing of
> the input and the writing of the data in the model is the
> responsibility of the controller, I think.

Yes, the view specific logic belongs in the controller. But I think it 
could make sense to have the generic view-independant "create a PR" 
functionality in the model. Input parsing is probably closely related to 
the view; when you use the API you are kind of providing values that 
already have been parsed.

But ok, there is probably some more high level input validation and user 
friendly error messages that perhaps should be shared and doesn't belong 
in the model. It is however something that establishes some sanity / 
consistency in the database so it could also be considered a part of the 
model.

Anyway, I think we are close to the justification of having both "pure 
json-rpc API" and the more ad-hoc json exposure of some view specific 
controllers.

>
>>> How to proceed here?
>>> Should I remove the NotAnonymous decorator and pass the requested user
>>> as argument to the API call?
>>
>> I am not sure in what way NotAnonymous and LoginRequired really are
>> different. For example, it seems weird that NotAnonymous doesn't consider
>> the IP filtering. We should perhaps use LoginRequired everywhere? Or would
>> this checking have too much overhead? I don't know. Try!
> I'm also not sure what the purpose of NotAnonymous is. It boils down
> to the purpose of the default user: why do we need it?
> If you're not logged in, does Kallithea treat you as the default user?

Yes, pretty much.

> Why would it be too much overhead? Just to check the IP against a list
> of IPs or subnets?
>
> Moreover, all cases where NotAnonymous is used also use the
> LoginRequired decorator, with the exception of
> ReposController:create and ReposController:create_repository.
>
> (grep -rn -C1 NotAnonymous kallithea/ )

Try it and see if the refactoring or testing gives more of an answer.

/Mads