koopjs / koop-cache-memory

Deprecated
Other
0 stars 4 forks source link

CRS is not cached #18

Closed guushub closed 1 year ago

guushub commented 2 years ago

Hi there,

I noticed that when I enabled the default in memory cache by setting ttl on a geojson, the CRS object is not stored in the cache. This can be quite an issue for data in my region (Europe). The geometries of these regions are usually not stored as EPSG:4326. So now the output defaults to 4326 after retrieving the features from the cache. The CRS object in the geojson is quite vital to project the data correctly for me and probably anyone using this with data thats projected and not stored in wgs84 coordinates.

I've tried working around this by using the 'after' option in registering the provider as documented here , but that does not seem to fix it. And I think this would be more of a workaround instead of a solution.

So I think it might be best to solve it in the koop-cache-memory library? I wouldn't mind contributing and fixing this myself, but I will need a pointer in the best way to approach it. I understand how it works, but I guess there needs to be a decission where to store it exactly; something like this.catalog.crs= new Map() probably?

Let me know if I can help! Thanks in advance.

Cheers, Guus

rgwozdz commented 2 years ago

@guushub - thanks very much for reaching out. I'll plan to step through this in the coming day so I can understand exactly where this is going wrong. I'll report back with thoughts on the best way to solve.

guushub commented 2 years ago

Great, thanks @rgwozdz !

rgwozdz commented 2 years ago

Hi @guushub - I think the thing to do is re-work how we insert/update as well as the retrieve and other methods. The cache retrieve should always return an exact copy of the data returned by the request that initially loaded the cache (unless we've appended features in the interim). Right now, that is not happening. Instead, prior to insert/upsert, the GeoJSONs features and metadata are being extracted and stored separately. Then they are reassembled on the retrieve operation. I guess this was added so you could get the metadata separately. That leaves things like crs or anything else at the root level of the GeoJSON orphaned. On the retrieve operation, features and metadata are acquired separately and then packaged up into a new GeoJSON wrapper.

I think there's a better way to go here. I think the insert/update methods should store the whole GeoJSON object:

Cache.prototype.insert = function (key, geojson, options = {}, callback = noop) {
  if (this.store.has(key)) {
    return callback(new Error('Cache key is already in use'))
  }
  this.store.set(key, geojson)
  const metadata = geojson.metadata || {}
  if (options.ttl) metadata.expires = Date.now() + (options.ttl * 1000)
  this.catalog.insert(key, metadata, callback)
}

There's going to be other changes required on the update, retrieve, append, and createStream methods. But all looks doable. Is this something you want to try to PR?

guushub commented 2 years ago

Hi @rgwozdz,

yes, that makes sense. I will take a look at it soon! In your example you still keep the metadata separate as well. Any particular reason that has your preference? Is it a requirement for the caching providers?

I will also update the tests and add a test to check for crs after retrieval.

Thanks for the update!

Cheers, Guus

rgwozdz commented 2 years ago

In your example you still keep the metadata separate as well. Any particular reason that has your preference? Is it a requirement for the caching providers?

Yes, note that the metadata is being cached separately by the catalog methods. These methods allow koop to fetch cached metadata by itself.