matterhorn-chat / mattermost-api Goto Github PK

To facilitate quick prototyping the library routines currently bailout directly. This is a bad design. In some cases we should be using exceptions and in other cases we should use Maybe or Either. This issue is a reminder to make the changes.

At the moment many of the functions of the form mmFoo return something of the form MM ([Header], Value). This exposes clients of the API to the fact that we use HTTP and JSON to communicate with the server. It would be better if we abstracted over these details.

We should make an effort to call fromJSON on the Value before returning it and the user shouldn't need the headers. It should be enough to assert that we got a positive response from the other end (200 or similar). Any meaningful headers should be reinterpreted and returned as a flag of some sort.

There is at least one thing that will make it hard to hide all the JSON details: We don't know how to interpret all the fields of the JSON blobs that we get back from mattermost. This will take some experimentation and reading of the mattermost code to figure out. As a compromise we should make an effort to convert all the top level types that we return into our own types. If there are fields we don't currently understand, we can either leave those uninterpreted (as strings) or Aeson's Value type (but only as a last resort).

The following new functions need tests:

• mmUpdatePost
• mmDeletePost

In the latest release of websockets the WebSocketsData class has a new method fromDataMessage that we need to implement.

When sending a time value to the server, an integer number of milliseconds is sent. This is generated as an Int by the timeToServer function. The possible range of this value should be determined for the APIv4 work and the resulting type should probably also have a validation on the conversion (although it could be argued that there's nothing the client could do in an overflow situation, but the valid range for the current API version should at a minimum be documented in the conversion).

As part of our effort to not expose users to the details of our dependencies we should translate exceptions to something that is meaningful to users of our API.

Mattermost 3.6 deprecated some API endpoints that will later be removed. The details can be found here:

https://docs.mattermost.com/administration/changelog.html#api-changes-from-v3-5-to-v3-6

The tsDebug field of TestState needs to be configurable with a command-line flag.

If Travis is present, run the tests with debugging enabled.

To reproduce:

$ cabal repl -b mattermost-api -w ghc-9.0.2 --constraint="aeson==2.0.*"
Resolving dependencies...
Build profile: -w ghc-9.0.2 -O1
In order, the following will be built (use -v for more details):
 - mattermost-api-50200.11.0 (lib) (requires build)
 - fake-package-0 (lib) (first run)
Starting     mattermost-api-50200.11.0 (lib)
Building     mattermost-api-50200.11.0 (lib)

Failed to build mattermost-api-50200.11.0.
Build log (
/home/ryanglscott/.cabal/logs/ghc-9.0.2/mattermost-api-50200.11.0-910294dc0d326ddcfd6e365178307af732845a8e719f4e94c8fdc8868174cb28.log
):
Configuring library for mattermost-api-50200.11.0..
Preprocessing library for mattermost-api-50200.11.0..
Building library for mattermost-api-50200.11.0..

<snip>

[12 of 17] Compiling Network.Mattermost ( src/Network/Mattermost.hs, dist/build/Network/Mattermost.o, dist/build/Network/Mattermost.dyn_o )

src/Network/Mattermost.hs:1068:42: error:
    • Couldn't match type: Data.Aeson.KeyMap.KeyMap Value
                     with: HashMap k0 Value
      Expected: HashMap k0 Value
        Actual: aeson-2.0.3.0:Data.Aeson.Types.Internal.Object
    • In the second argument of ‘HM.lookup’, namely ‘o’
      In the expression: HM.lookup "message" o
      In the expression:
        case HM.lookup "message" o of
          Just (String msg)
            -> let newMsg = ... in throwIO $ MattermostServerError newMsg
          _ -> throwIO $ httpExc
     |
1068 |                 case HM.lookup "message" o of
     |                                          ^
cabal: Failed to build mattermost-api-50200.11.0 (which is required by
fake-package-0). See the build log above for details.

I originally discovered this when trying to build mattermost-api with GHC 9.2, as 9.2 requires aeson-2.0.* or later.

(I'm working on this right now, so this is a test issue to make sure it's working.)

Our dependencies are all over the place in terms of expecting String vs. Text vs. ByteString (and their lazy variants). At the moment we almost exclusively expose String in our types. The big exception would be the Id type. String is a fairly non-controversial choice but it might be better still to use Text everywhere.

Need to add logging for WebSocket errors/exceptions. It may also be advisable to have more specific handling of issues.

We need to have prominent documentation about how to interpret our version strings so users know how to choose versions of this software.

These are currently hardcoded to 0 and 10000.

The MM monad that we have is a good example of how someone might wrap up the API, but I'm now leaning towards that being an abstraction that we let the application programmer choose for themselves.

So here is the proposal:

• Take the MM monad out of base API and have the API explicitly take the auth token as a param and return types should be like IO (Either String Foo) or use IO exceptions like IO Foo.
• In the examples, provide an example MM monad that makes the API easier to use.

Basically, consumers of the API might want to use their own monad stack they might want to use mtl or monadLib instead of whatever we pick. So the point should be make it easy for people to wrap the base API up in whatever monad stack they want. We could even provide an example wrapped API as a separate package, but the API in this package should be the "raw" API.

In discussions about versioning schemes we concluded that a good approach would be to set library versions to ABBCC.X.Y as an encoding of the supported MatterMost server version A.B.C (with single-digit B and C components left-padded with 0) with X.Y used to represent the library evolution / bugfix status w.r.t. the encoded server version.

Add tests for websocket functionality.

There are lots of times when it would be nice to increase logging in various ways. A common problem is having a json object not decode as expected or a field to be set in an unexpected way.

Right now these functions are duplicates except for one part of the route string. We should refactor them to remove the duplication.

As best I can tell, the UserProfile data type is not needed and all API calls having to do with users should use User instead. Due to #22 I haven't been able to verify this experimentally, but the API docs suggest this is the case because they do not distinguish between "users" and "user profiles."

Looks like the travis build on ghc 7.10 is failing in the websocket package:

src/Network/WebSockets/Hybi13.hs:23:62:
    Module ‘Data.Binary.Get’ does not export ‘getInt64be’
Failed to install websockets-0.11.2.0

I don't know if this is an issue with the way our dependencies are specified or if websockets just doesn't work with 7.10 anymore. I do suspect that the issue can be resolved by being more restrictive about the version of binary that we use allow on 7.10.

Hi Folks,

I've build the current master branch of mattermost-api and the matterhorn client.

% cabal install
Resolving dependencies...
Notice: installing into a sandbox located at
/home/user/dev/matterhorn/.cabal-sandbox
Downloading aspell-pipe-0.3...
Downloading mattermost-api-40000.0.1...
Configuring mattermost-api-40000.0.1...
Configuring aspell-pipe-0.3...
Building mattermost-api-40000.0.1...
Building aspell-pipe-0.3...
Installed aspell-pipe-0.3
Installed mattermost-api-40000.0.1
Configuring matterhorn-40000.0.2...
Building matterhorn-40000.0.2...
Installed matterhorn-40000.0.2
cabal install  77.06s user 1.78s system 100% cpu 1:18.40 total

When running the client I get a JSONDecodeException:

% matterhorn
Authenticating...
Loading channels for team "team"...
matterhorn: JSONDecodeException {jsonDecodeExceptionMsg = "Error in $.event: 
Unknown websocket message: \"channel_viewed\"", jsonDecodeExceptionJSON = 
"{\"event\":\"channel_viewed\",\"data\":{\"channel_id\":\"n1qjsjhqwfbedesc7xgtenm5ch\"},
\"broadcast\":{\"omit_users\":null,\"user_id\":\"fbgzd3r3eige7nykphqb5jf86e\",
\"channel_id\":\"\",\"team_id\":\"\"},\"seq\":2}"}

Any Idea what might be wrong?
How can I provide further debug information?

Thanks.

I figure this might be the right place to report this. I tried running the demo client and got this after allowing access to my password:

cygnus@pickle:~/src/mattermost-demo-client$ .cabal-sandbox/bin/mattermost-demo-client
mattermost-demo-client: JSONDecodeException {jsonDecodeExceptionMsg = "mmGetJSONBody: Error in $: key "last_picture_update" not present", jsonDecodeExceptionJSON = "{"id":"en4wx66nnty75j63irwqzz41cr","create_at":1461001853400,"update_at":1466632136323,"delete_at":0,"username":"cygnus","auth_data":"","auth_service":"ldap","email":"[email protected]","email_verified":true,"nickname":"","first_name":"Jonathan","last_name":"Daugherty","roles":"","last_activity_at":1471926878355,"last_ping_at":1471926885728,"notify_props":{"all":"true","channel":"true","desktop":"none","desktop_sound":"false","email":"false","first_name":"false","mention_keys":"cygnus,@cygnus"},"last_password_update":1461001853400,"locale":"en"}"}

At present, the testing process for this library assumes that the mattermost-preview image is appropriate for testing. While I think having a build against that image is very useful to find out about upcoming release incompatibility, it breaks our builds when it gets updated because we don't have a way of testing against the current release. So the task for this ticket is to figure out whether there exists a production docker image of mattermost that we can use for our testing when that's what we want, and then update the testing script to choose which image to use.

Currently hardcoded to limit of 10000 and offset of 0.

A few endpoints still return a raw aeson Value instead of a proper type. We should fix that.

Right now when the test suite fails it show you json blobs that have already been through Show and so the strings all end up escaped. It's a mess to read as a result.

Currently, each HTTP request opens a new connection to the server. This causes a noticeable lag on slow internet connections.
A possible solution is to implement pooling for HTTP connections using a library like https://hackage.haskell.org/package/resource-pool.

In some places we give the API end points names that make sense to us and in other places we have prioritized names that match the URLs used. I think it would be best to use the nomenclature of the MatterMost folks but document our understanding as well.

This issue is mostly a place holder to remind us to go through and deliberately rename things. After that initial pass we should close this ticket and document the rules for adding new end points.