Extract functionality for submitting collected data
Needs ReviewPublic

Authored by vitalyf on Apr 1 2019, 9:43 AM.

Details

Reviewers
vkrause
Summary
  • Added "interface" AbstractDataSubmitter which is the base class for

all data sending strategies.

  • Added "interface" AbstractJsonDataSubmitter which is the base class

for all JSON sending strategies.

  • Existing data sending functionality is extracted to the

DefaultDataSubmitter class.

  • Simplified existing solution: used built-in redirects resolving

instead of manually implemented.

  • Qt 5.9 is required.

Diff Detail

Repository
R849 User Feedback Collection Framework
Lint
Lint Skipped
Unit
Unit Tests Skipped
vitalyf requested review of this revision.Apr 1 2019, 9:43 AM
vitalyf created this revision.

Didn't have time to look into (1) and (2) yet, but the reason for (3) is compatibility all the way back to Qt 5.5, which is oldest version still supported by GammaRay.

Didn't have time to look into (1) and (2) yet, but the reason for (3) is compatibility all the way back to Qt 5.5, which is oldest version still supported by GammaRay.

Oh, I see. Probably it's a good time to move GammaRay to 5.12 as this is LTS?
Unfortunately, code without automatic redirects handling looks tangled.

Didn't have time to look into (1) and (2) yet, but the reason for (3) is compatibility all the way back to Qt 5.5, which is oldest version still supported by GammaRay.

Oh, I see. Probably it's a good time to move GammaRay to 5.12 as this is LTS?
Unfortunately, code without automatic redirects handling looks tangled.

I wish! We just now dropped support for Qt 4.8 - 5.4 for the upcoming release, anything above that still has an active user base on current GammaRay releases (which we know thanks to telemetry).

I like the general approach of factoring out the data submission, and making that an extension vector. I am however unsure about how that interface should look like. This now exposes very fine grained switches (like setting of certain HTTP headers) and protocol internals (the probing GET before POSTing, to support redirects), which feels like we are doing this at a too low level. I can't imagine the presence of a content length header being the decisive difference in practice for example.

Shouldn't this rather be on the level of "here's a JSON document, a server URL and product id, submit it and let me know how that went"? That is, leave all the details of HTTP headers, probing requests, etc to the actual implementation? One could even argue that no QtNetwork usage should leak through that interface, even if that is probably unnecessarily extreme.

vitalyf added a comment.EditedApr 2 2019, 9:47 AM

I like the general approach of factoring out the data submission, and making that an extension vector. I am however unsure about how that interface should look like. This now exposes very fine grained switches (like setting of certain HTTP headers) and protocol internals (the probing GET before POSTing, to support redirects), which feels like we are doing this at a too low level. I can't imagine the presence of a content length header being the decisive difference in practice for example.

Shouldn't this rather be on the level of "here's a JSON document, a server URL and product id, submit it and let me know how that went"? That is, leave all the details of HTTP headers, probing requests, etc to the actual implementation? One could even argue that no QtNetwork usage should leak through that interface, even if that is probably unnecessarily extreme.

Well, this is more matter of customization abilities. For example, some user for some reasons might want to send not JSON w/o length in a future. Output data generator should be set as a separate strategy as well and allow user to specify a different output format, for now it's only JSON... But what if someone want to send XML or just random BLOB? It should be another customization point. We don't need it now, but it's highly possible.
That's also a reason why setting only url and doc wouldn't work. What if I need to set, say, some auth headers/data. Or dynamically change submission URL based on something.
Regarding QNetwork usage, well, I think it's good idea to leak it through the interface. Because:

  1. This is already high level abstraction around requests
  2. You can configure a lot of things, for example, redirects, query, headers and so on
  3. We always be using Qt, hence, another abstraction level for different framework is not required

Summarizing, I think that this is good customization level. I already implemented two classes (standard submitter and one for our plugin) based on this interface, and I would say that this is convenient.

I like the general approach of factoring out the data submission, and making that an extension vector. I am however unsure about how that interface should look like. This now exposes very fine grained switches (like setting of certain HTTP headers) and protocol internals (the probing GET before POSTing, to support redirects), which feels like we are doing this at a too low level. I can't imagine the presence of a content length header being the decisive difference in practice for example.

Shouldn't this rather be on the level of "here's a JSON document, a server URL and product id, submit it and let me know how that went"? That is, leave all the details of HTTP headers, probing requests, etc to the actual implementation? One could even argue that no QtNetwork usage should leak through that interface, even if that is probably unnecessarily extreme.

Well, this is more matter of customization abilities. For example, some user for some reasons might want to send not JSON w/o length in a future. Output data generator should be set as a separate strategy as well and allow user to specify a different output format, for now it's only JSON... But what if someone want to send XML or just random BLOB? It should be another customization point. We don't need it now, but it's highly possible.

My assumption would be that you can customize all that in the specific implementation. If the entire network communication is inside the data submitter implementation you have unlimited control over HTTP headers and the format you send (a JSON document might be the wrong type in the interface then, let's assume a variant map instead).

Simplifying the interface therefore doesn't limit us, it actually gives us more flexibility, by not forcing things upon a data submitter implementation that just currently happen to make sense for both backends.

That's also a reason why setting only url and doc wouldn't work. What if I need to set, say, some auth headers/data. Or dynamically change submission URL based on something.

That's exactly why I want to simplify the interface, we cannot guess every possible protocol customization that will ever be needed, so let's not expose that in the interface. If you need authentication, your submitter implementation can set whatever it needs on the network requests it creates. If you need to set any other specific header you can do that too, without being limited by flags of the base class.

Regarding QNetwork usage, well, I think it's good idea to leak it through the interface. Because:

  1. This is already high level abstraction around requests
  2. You can configure a lot of things, for example, redirects, query, headers and so on
  3. We always be using Qt, hence, another abstraction level for different framework is not required

I'm not suggesting to abstract the QtNetwork operations, that is indeed pointless, but abstracting the one operation we expect the backend to do: submit a given set of data.

Summarizing, I think that this is good customization level. I already implemented two classes (standard submitter and one for our plugin) based on this interface, and I would say that this is convenient.

So, you propose interface like this:

class AbstractDataSubmitter : public QObject
{
public:
    ~AbstractDataSubmitter() override;

    // QByteArray is a universal type to represent pretty much everything. Creating data for sending is a responsibility of another class, I would say.
    virtual void submit(const QByteArray &data) const;

    QUrl serverUrl() const;
    QString productId() const;

public Q_SLOTS:
    void setProductId(QString id);
    void setServerUrl(QUrl url);

Q_SIGNALS:
    void dataSubmissionFinished(const DataSubmissionResult &result) const;
};

In order to give a user more ability to customize the process. Am I right?
This actually would work, but I need to perform some tests.

So, you propose interface like this:

class AbstractDataSubmitter : public QObject
{
public:
    ~AbstractDataSubmitter() override;

    // QByteArray is a universal type to represent pretty much everything. Creating data for sending is a responsibility of another class, I would say.
    virtual void submit(const QByteArray &data) const;

    QUrl serverUrl() const;
    QString productId() const;

public Q_SLOTS:
    void setProductId(QString id);
    void setServerUrl(QUrl url);

Q_SIGNALS:
    void dataSubmissionFinished(const DataSubmissionResult &result) const;
};

In order to give a user more ability to customize the process. Am I right?
This actually would work, but I need to perform some tests.

Exactly, I'm much more happy with that :)

The data encoding can be done in a separate class, or alternatively in here too (by changing the input to something structured like variant map or JSON). Separating seems somewhat cleaner on first sight, but OTOH I expect both to be strictly tied together, arbitrarily mixing encoding and transmission will not work. Combining this therefore will remove a source of mistakes for the user. Anyway, I can live with both approaches for this.

@vkrause, I'm thinking about adding one more basic class for sending data. This class will inherit the interface we discussed above, but also will provide some common utilities based on QtNetwork things (I think, it should be functionality, we decided to remove from basic class). I propose name like "BaseDataSubmitter" or "QtBasedDataSubmitter". We need this, to avoid massive code duplication.
What do you think about this idea?

@vkrause, I'm thinking about adding one more basic class for sending data. This class will inherit the interface we discussed above, but also will provide some common utilities based on QtNetwork things (I think, it should be functionality, we decided to remove from basic class). I propose name like "BaseDataSubmitter" or "QtBasedDataSubmitter". We need this, to avoid massive code duplication.
What do you think about this idea?

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

@vkrause, I'm thinking about adding one more basic class for sending data. This class will inherit the interface we discussed above, but also will provide some common utilities based on QtNetwork things (I think, it should be functionality, we decided to remove from basic class). I propose name like "BaseDataSubmitter" or "QtBasedDataSubmitter". We need this, to avoid massive code duplication.
What do you think about this idea?

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

Only requests, I would say. The new class should look like this:

enum class SubmissionOption { ... }

class BaseDataSubmitter : public AbstractDataSubmitter
{
public:
     void submit(const QByteArray &data) const final;

    SubmissionOptions options() const;
    void setOptions(KUserFeedback::SubmissionOptions options);

protected:
   virtual QNetworkRequest probeRequest() const;
   virtual QNetworkRequest dataRequest() const = 0;

   // ...
};

Basically, what I propose is to split AbstractDataSubmitter I implemented before, to two classes: more abstract interface and more specific implementation to keep common and useful functionality. If a user need to implement something specific, he could take the top-level interface class AbstractDataSubmitter, if it would be enough to just slightly configure requests, he could take BaseDataSubmitter (or we can name it QtBasedDataSubmitter).

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

Only requests, I would say. The new class should look like this:

enum class SubmissionOption { ... }

class BaseDataSubmitter : public AbstractDataSubmitter
{
public:
     void submit(const QByteArray &data) const final;

    SubmissionOptions options() const;
    void setOptions(KUserFeedback::SubmissionOptions options);

protected:
   virtual QNetworkRequest probeRequest() const;
   virtual QNetworkRequest dataRequest() const = 0;

   // ...
};

Basically, what I propose is to split AbstractDataSubmitter I implemented before, to two classes: more abstract interface and more specific implementation to keep common and useful functionality. If a user need to implement something specific, he could take the top-level interface class AbstractDataSubmitter, if it would be enough to just slightly configure requests, he could take BaseDataSubmitter (or we can name it QtBasedDataSubmitter).

Or maybe [Abstract]RestDataSubmitter? As you said earlier, any implementation is likely based on Qt :) Having virtual methods to get the requests makes sense, and is a manageable public interface (which I assume is what this will be, as your implementation will be external, right?). I'm wondering though if we can avoid the submission options.

That contains 3 options from what I can see:

  • add mimetype and payload size header fields: if you need to customize that you could adjust it in the xRequest() methods too, right? Same as e.g. adding an API key header. I don't like these options as they well overly specific, and I don't even think our current backend cares much about those headers being present or not, ie. if you need something specific for your backend, maybe we can just set that as default?
  • enabling the probe request: that makes more sense on first sight, however the reason why this was added is that redirects on POST is apparently somewhat undefined in the HTTP standard, and quite inefficient as you send the full payload just to learn you have to do so again. And redirection support is something you most likely want in long-term deployments, to be able to adjust your infrastructure on the server over time. If you want to avoid that, how are you going to deal with this issue instead?

Sharing common bits of network code makes perfect sense of course. Not knowing your backend interface makes it hard to suggest specifics though. What is the main difference to the current interface, ie. which parts do you actually need to be able to control?

Only requests, I would say. The new class should look like this:

enum class SubmissionOption { ... }

class BaseDataSubmitter : public AbstractDataSubmitter
{
public:
     void submit(const QByteArray &data) const final;

    SubmissionOptions options() const;
    void setOptions(KUserFeedback::SubmissionOptions options);

protected:
   virtual QNetworkRequest probeRequest() const;
   virtual QNetworkRequest dataRequest() const = 0;

   // ...
};

Basically, what I propose is to split AbstractDataSubmitter I implemented before, to two classes: more abstract interface and more specific implementation to keep common and useful functionality. If a user need to implement something specific, he could take the top-level interface class AbstractDataSubmitter, if it would be enough to just slightly configure requests, he could take BaseDataSubmitter (or we can name it QtBasedDataSubmitter).

Or maybe [Abstract]RestDataSubmitter? As you said earlier, any implementation is likely based on Qt :) Having virtual methods to get the requests makes sense, and is a manageable public interface (which I assume is what this will be, as your implementation will be external, right?). I'm wondering though if we can avoid the submission options.

That contains 3 options from what I can see:

  • add mimetype and payload size header fields: if you need to customize that you could adjust it in the xRequest() methods too, right? Same as e.g. adding an API key header. I don't like these options as they well overly specific, and I don't even think our current backend cares much about those headers being present or not, ie. if you need something specific for your backend, maybe we can just set that as default?
  • enabling the probe request: that makes more sense on first sight, however the reason why this was added is that redirects on POST is apparently somewhat undefined in the HTTP standard, and quite inefficient as you send the full payload just to learn you have to do so again. And redirection support is something you most likely want in long-term deployments, to be able to adjust your infrastructure on the server over time. If you want to avoid that, how are you going to deal with this issue instead?

Well, yes. We can avoid using options and set all headers and so on in specific request methods. This would work, I think.
Regarding probe request, I see your point. We don't actually need a separate option for it. If default implementation returns invalid probe request, it just won't be sent.
Regarding redirects handling, I suggest just use mechanism introduced in Qt 5.6. That one I already implemented, but this of course will be moved to the second class which is more specific.

Regarding redirects handling, I suggest just use mechanism introduced in Qt 5.6. That one I already implemented, but this of course will be moved to the second class which is more specific.

5.6 or 5.9? As mentioned before with GammaRay using this an needing 5.5 support this is a problem. If we are talking about 5.6 I could possibly just update the snapshot GammaRay uses to before this change and hope we wont need another update before GammaRay doesn't support 5.5 anymore. For 5.9 this is more of an issue though.

vitalyf updated this revision to Diff 55806.Apr 9 2019, 10:09 AM
vitalyf edited the summary of this revision. (Show Details)

Regarding redirects handling, I suggest just use mechanism introduced in Qt 5.6. That one I already implemented, but this of course will be moved to the second class which is more specific.

5.6 or 5.9? As mentioned before with GammaRay using this an needing 5.5 support this is a problem. If we are talking about 5.6 I could possibly just update the snapshot GammaRay uses to before this change and hope we wont need another update before GammaRay doesn't support 5.5 anymore. For 5.9 this is more of an issue though.

Yes, you're right it's 5.9.
BTW, I've updated the diff.

Conceptually and structurally I think this is getting there, but we need to address the minimum Qt requirement :) Right now even 5.12 isn't enough due to the Q_DISABLE_COPY_MOVE it seems, so I can't easily test this here.

Conceptually and structurally I think this is getting there, but we need to address the minimum Qt requirement :) Right now even 5.12 isn't enough due to the Q_DISABLE_COPY_MOVE it seems, so I can't easily test this here.

Oh, I see! We definitely need to come up with minimum Qt requirements. Looks like I used Qt 5.13, the latest available I have. You can just comment out Q_DISABLE_COPY_MOVE to build. I'll remove it from the code.

Conceptually and structurally I think this is getting there, but we need to address the minimum Qt requirement :) Right now even 5.12 isn't enough due to the Q_DISABLE_COPY_MOVE it seems, so I can't easily test this here.

Oh, I see! We definitely need to come up with minimum Qt requirements. Looks like I used Qt 5.13, the latest available I have. You can just comment out Q_DISABLE_COPY_MOVE to build. I'll remove it from the code.

Right, without that it builds with 5.12. Most tests pass still, submittest is broken though, due to no submitter being set. We should create the default one in that case I think?

Regarding the minimum version, that is 5.5 as said previously, and as mentioned in our call with Frederik a while ago.

vitalyf updated this revision to Diff 55978.EditedApr 11 2019, 9:56 AM

I've fixed the test and also removed 5.13 introduced code.

Would it make sense to create the default submitter automatically inside Provider if none is set? That wouldn't break any of the existing users, as I suspect both the UserFeedbackConsole application and the examples are now also not working anymore, and it shouldn't interfere with what you are needing I guess?

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this. There's smaller things like pragma once vs. include guards, but this can easily be fixed later as well.

vitalyf updated this revision to Diff 56041.Apr 12 2019, 8:37 AM

DefaultDataSubmitter is set by default. I also changed "#pragma once" to include guards.

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Hi @vkrause, Any luck here?

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Hi @vkrause, Any luck here?

If I'm not mistaken this patch still raises the required Qt version from 5.5 to at least 5.9, right? If so that still needs to be addressed (as in: restore Qt 5.5 compatibility) before this can be integrated. I'm sorry if that wasn't clear.

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Hi @vkrause, Any luck here?

If I'm not mistaken this patch still raises the required Qt version from 5.5 to at least 5.9, right? If so that still needs to be addressed (as in: restore Qt 5.5 compatibility) before this can be integrated. I'm sorry if that wasn't clear.

Oh, I see. Sorry for misunderstanding, I thought that you are going to move to a newer Qt... BTW, are there any plans to do that this year or would it be better if I change this patch?

Apart from that I think we only need to resolve the minimum Qt version issue before integrating this.

Hi @vkrause, Any luck here?

If I'm not mistaken this patch still raises the required Qt version from 5.5 to at least 5.9, right? If so that still needs to be addressed (as in: restore Qt 5.5 compatibility) before this can be integrated. I'm sorry if that wasn't clear.

Oh, I see. Sorry for misunderstanding, I thought that you are going to move to a newer Qt... BTW, are there any plans to do that this year or would it be better if I change this patch?

I wish we could, but I don't see that happening before probably the Qt6 time, the old versions are still in active use by a relevant amount of GammaRay users, and I had a hard time arguing for dropping 4.8 - 5.4 already :-(
So I'm afraid we need to adjust the patch, sorry.