matomo-org / matomo-java-tracker

Official Java implementation of the Matomo Tracking HTTP API.
https://matomo-org.github.io/matomo-java-tracker/
BSD 3-Clause "New" or "Revised" License
69 stars 52 forks source link

Action and Download URLs should support more protocols #15

Closed rivoduck closed 8 years ago

rivoduck commented 8 years ago

ACTION_URL and DOWNLOAD_URL are defined as java.net.URL.URL objects in the getters and setters. I think they should be set and get as strings in order to accept more protocols than the ones supported by the URL class. This for example would allow tracking of RTMP URLs.

TODO: verify if Piwik accepts RTMP and other non-HTTP protocols!

Thank you, Andrea

bcsorba commented 8 years ago

Hi Andrea,

Trying out a few experiments, it seems like Piwik will accept a subset of protocols (including RTMP) for fields such as ACITON_URL, but will not recognize certain other protocols and arbitrary strings. @mattab would you be able to point us to some documentation about what formats are supported by fields such as "url" and "download"? If there is a defined type I would like to accept that as the method parameters, to prevent individuals from submitting unsupported strings and not seeing their results displayed in Piwik.

rivoduck commented 8 years ago

Hi Brett,

I am using your classes to track video streaming access. I modified your code to deal with URLs as strings to allow for more flexibility. During my tests I passed URLs with arbitrary strings (eg. RtspServer1:://server1/ and Piwik was able to process and display them without problems.

I modified PiwikRequest.java, here are the methods I changed:

    8a9,11
    > 
    > 
    > 
    123c126
    <     public PiwikRequest(Integer siteId, URL actionUrl){
    ---
    >     public PiwikRequest(Integer siteId, String actionUrl){
    179,180c182,183
    <     public URL getActionUrl(){
    <         return (URL)getParameter(ACTION_URL);
    ---
    >     public String getActionUrl(){
    >         return (String)getParameter(ACTION_URL);
    187c190
    <     public void setActionUrl(URL actionUrl){
    ---
    >     public void setActionUrl(String actionUrl){
    496,497c499,500
    <     public URL getDownloadUrl(){
    <         return (URL)getParameter(DOWNLOAD_URL);
    ---
    >     public String getDownloadUrl(){
    >         return (String)getParameter(DOWNLOAD_URL);
    505c508
    <     public void setDownloadUrl(URL downloadUrl){
    ---
    >     public void setDownloadUrl(String downloadUrl){
    1065,1066c1068,1069
    <     public URL getReferrerUrl(){
    <         return (URL)getParameter(REFERRER_URL);
    ---
    >     public String getReferrerUrl(){
    >         return (String)getParameter(REFERRER_URL);
    1074c1077
    <     public void setReferrerUrl(URL refferrerUrl){
    ---
    >     public void setReferrerUrl(String refferrerUrl){

So far I did not find any problem with Piwik accepting the tracking data. It is a very impacting change since it requires refactoring the code that uses it!

Andrea

bcsorba commented 8 years ago

All URL based getters and setters now have a get...AsString and a set...WithString methods allowing users to input String values. Overloaded the original getter/setter methods and the constructor was not done to prevent breaking API changes when a "null" value was used.

mattab commented 8 years ago

. @mattab would you be able to point us to some documentation about what formats are supported by fields such as "url" and "download"? If there is a defined type I would like to accept that as the method parameters, to prevent individuals from submitting unsupported strings and not seeing their results displayed in Piwik.

Hi @bcsorba - our general policy is to try and store as much data as possible, so we do as little validation as possible. Therefore, I think it is best that the SDK forwards all URLs as Strings so that the requests still make it to the Tracking API and it is up to the tracking API to decide what to do. For example, some plugins may change the behavior of what is accepted for a given parameter. To summarise, the solution you implemented looks good :+1:

bcsorba commented 8 years ago

Released in v1.1