[Patchew-devel] [PATCH v2 3/3] Make Import Command use Rest

[Fam Zheng]

On Tue, 07/03 13:41, Shubham Jain wrote:
> On importing a message which is already imported before, the REST would
> give error UNIQUE constraint failed: api_message.project_id,
> api_message.message_id.

This breaks scripts/patchew-impoter. Upon network error, the importer instance
is restarted and will retry the import. The server may already received the
message but just failed to return to patchew-cli, in this case, an empty
project_list makes more sense than error.

The UNIQUE contraint error should only apply to explicit project list when
import. From a user perspective, if I submit a message and rely on the server to
figure out which projects to add it, I don't think such an error is reasonable,
even if it has been imported before.

Fam

> 
> On Tue, Jul 3, 2018 at 8:20 AM Fam Zheng <famz at redhat.com> wrote:
> 
> > On Mon, 07/02 18:39, Paolo Bonzini wrote:
> > > On 02/07/2018 10:27, Fam Zheng wrote:
> > > > On Mon, 07/02 13:40, Shubham Jain wrote:
> > > >> Right. So in REST we are giving permissions to import any
> > authenticated
> > > >> user but the projects would be only imported to recognised projects.
> > We can
> > > >> give warning in our client when there are no messages are imported.
> > > >
> > > > Or maybe it's even better to give warnings when projects recognize the
> > patch but
> > > > permission is lacking.
> > > >
> > > > But for the importer running in the background, making sure the exit
> > status is
> > > > not 0 when any error happens is the highest priority, this includes any
> > > > authentication and permission issues.
> > >
> > > An unauthenticated user will always return 1, because POST will fail
> > > with 403.  However, for an authenticated _but unauthorized_ user that
> > > sends /api/v1/messages/, the response will be successful.
> > >
> > > In patchew-cli, returning success is probably okay for a generic user,
> > > but indeed Fam is right, it is not okay for an importer that runs in the
> > > background.
> > >
> > > Shubham, please modify the patch to return an exit status of 1 when the
> > > project_list is empty, so that the background importer signals the error
> > > promptly.
> >
> > Do we return empty project_list if the message is already imported before?
> > If so
> > there should be no error. Similarly if the message format is valid but it
> > doesn't belong to any known project, it should be nop and no error.
> >
> > Fam
> >
> > >
> > > The API doesn't tell you which projects have not had permission, since
> > > it only returns (in the 201 Created response) the resources that were
> > > created.  However, the common case of "I forgot to put the user in the
> > > importer group" will be caught just fine.  Special users should not be
> > > assigned as maintainers!
> >
> >
> >