Backend cann't parse new response from tracker for newly generated key

[josecelano]

The tracker has an endpoint to generate new keys.

The response for that endpoint was like:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 1674804892
}

But now is like this:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 
    { 
      "secs": 1674804892, 
      "nanos": 423855037
    }
}

The change was introduced in this commit cab093c026cf253ba6f7208211953a25b3d1c91b by @da2ce7.

The struct:

pub struct AuthKey {
    pub key: String,
    pub valid_until: Option<u64>,
 }

changed to:

pub struct AuthKey {
    pub key: String,
    pub valid_until: Option<DurationSinceUnixEpoch>,
}

Since we automatically build the json objects exposed in the API, that change was exposed to API consumers.

The backend cannot parse the new response, and keys are not stored in the table torrust_tracker_keys. So the function TrackerService::retrieve_new_tracker_key is called every time we use the TrackerService.

[josecelano]

I've temporarily fixed the PR I'm working on.

[josecelano]

I suppose we should revert the API change instead of changing the backend. It does not make sense to return the nanoseconds.

[da2ce7]

@josecelano Woops! Makes it obvious that we need more endpoint tests... Or better: We should use json schemas with validation!

I think that the new format is better, however we should remove the nano field.

Something like:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 
    { 
      "secs": 1674804892
    }
}

[josecelano]

@josecelano Woops! Makes it obvious that we need more endpoint tests... Or better: We should use json schemas with validation!

I think that the new format is better, however we should remove the nano field.

Something like:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 
    { 
      "secs": 1674804892
    }
}

@da2ce7 yes, I think we could start adding a couple of high-level tests to catch as many regression bugs as possible.

Regarding the solution, maybe we can use struts instead of just converting whatever the struct is into a json:

// POST /api/key/:seconds_valid
// Generate new key
let t5 = tracker.clone();
let create_key = filters::method::post()
    .and(filters::path::path("key"))
    .and(filters::path::param())
    .and(filters::path::end())
    .map(move |seconds_valid: u64| {
        let tracker = t5.clone();
        (seconds_valid, tracker)
    })
    .and_then(|(seconds_valid, tracker): (u64, Arc<TorrentTracker>)| async move {
        match tracker.generate_auth_key(Duration::from_secs(seconds_valid)).await {
            Ok(auth_key) => Ok(warp::reply::json(&auth_key)),
            Err(..) => Err(warp::reject::custom(ActionStatus::Err {
                reason: "failed to generate key".into(),
            })),
        }
    });

We can change the converter warp::reply::json(&auth_key) and make it explicit.

That forces you to update the struct representing the json.

pub struct AuthKey {
    pub key: String,
    pub valid_until: Option<DurationSinceUnixEpoch>,
}

pub struct AuthKetDto {
    pub key: String,
    pub valid_until: i64
}

impl AuthKetDto {
    pub fn fromAuthKey(auth_key: &AuthKey) -> Self {
        let valid_until = match ...
        Self {
            key: auth_key.key;
            valid_until
        }   
    }
}

And then you can change this code:

warp::reply::json(&auth_key)

by

AuthKetDto::from(&auth_key).to_json() // or with a converter or whatever ...

Frameworks use that magic so you can change models and endpoint change automatically. I think for fast prototyping is OK, but I prefer to change it manually for stable APIs in production.

I think we can research for API contracts with strict types (for example https://github.com/flock-community/zod-endpoints).

With the solution above, I think we could be sure the endpoint is returning a given type. Some years ago, I went to a talk where they explained how they used the same types in the backend and front with Typescript to enforce the contract. I this the contract were those types, and they were located in an independent package used by the back and front.

In fact, it forces the backend to use a concrete version of the "contract", and it's a way for versioning the API, which is something we also have to think about how to solve.

I suppose that solution makes it harder to have endpoints that can respond to different versions of the API, because you cannot install different versions of the contract. The package should contain all the versions in a way that the API version and the package version are not the same. This solution is probably better for internal APIs.

NOTE about terminology:

• Internal API: when you are the consumer of your API. For example, an API for your SPA.
• External API: when you do not know your consumers.

I suppose we are in the second case, as we discussed some weeks ago.

And finally, regarding the json schema you proposed, I prefer this one:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until_timestamp": 1674804892
}

I generally like putting a suffix with the units (seconds, meters, UTC, ...).

In your case, it might be confusing if we refer to the duration in seconds (although that would be valid_for instead of valid_until).

But I think using the _timestamp prefix is clearer and implies we are using seconds. And having only one field, I do not see the need to add an extra level.

On the other hand, I have the feeling that datetime in UTC is the most standard way to represent time and is more readable. Json is supposed to be readable by humans. With a datetime maybe you do not even need to add a prefix:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": "2015-05-22T14:56:28.000Z"
}

because it's obvious that it's a date.

[da2ce7]

Yes, this format is the best. Date-Times should be transferred in UTC format, not the internal Unix format.

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": "2015-05-22T14:56:28.000Z"
}

[josecelano]

The API change was reverted here.

We can remove the struct:

pub struct NewTrackerKey {
    pub key: String,
    pub valid_until: Duration,
}

in src/models/tracker_key.rs.

@da2ce7 should we add the new field in the tracker API response?

If we want to do it, we can do it in this issue, but we need to create a new issue on the tracker repo. I suggest doing it in parallel, like the other API change. We can keep the current name until the new major version release. Something like:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "valid_until": 1674804892                         -> Mark as deprecated
  "key_valid_until": "2015-05-22T14:56:28.000Z"         
}

I propose to use the name key_valid_until for the new field. So in the future, we will have:

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "key_valid_until": "2015-05-22T14:56:28.000Z"         
}

or

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "expires_on": "2015-05-22T14:56:28.000Z"         
}

@da2ce7 @WarmBeer, if you think this is not a priority, we can remove the NewTrackerKey struct to close this issue.

[da2ce7]

Best to be explicit:

POST /api/key/:expiry_time

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "expiry_time": "2015-05-22T14:56:28.000Z"         
}

[josecelano]

Best to be explicit:

POST /api/key/:expiry_time

{
  "key": "IaWDneuFNZi8IB4MPA3qW1CD0M30EZSM",
  "expiry_time": "2015-05-22T14:56:28.000Z"         
}

OK. I've created the issue on the tracker.

We can close this issue via the PR, removing the unused struct. And open it again to remember we have to change the backend when we finish the issue.