Repository navigation
fix(engine): keep a cloud-optimised GeoTIFF's look in a saved project - #1533
elpidioassinaturas wants to merge 4 commits into
Conversation
A plain web address lost everything after `#` when saved, so a COG lost its `#range=…` and `#bands=…`, a data cube lost its fixed range on every date, and saving counted each address as a removed key. Only a look stays: any other fragment still goes, as OAuth and SAS-style links carry tokens there. `dequantize` in a look now takes only `aef` or `none`, since a look is saved as written and must not hold free text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Code reviewI traced Bugs
Security
Performance
Quality
AGENTS.md
Change sizeCompared with the base branch (merge base 3c119b7 to PR head 159523b), the whole PR.
|
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
…reading Tightening `Look::parse` also changed how an address is read: a hand-typed `#range=0,1&dequantize=other` lost its range, unsaid. Reading stays as it was; `split_look` now refuses to keep a look whose `dequantize` is another word. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aved-address-look
After opengeos#1339 a look also has `cube`, which takes any word as `dequantize` does. `split_look` now names what may stay, so a key added later isn't saved as written until it is named here. A one-file cube keeps its `#cube=1`, the only thing that keeps it a tile layer when its project opens. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Left from the review summary above, for the release quality pass: the temp directory in |
Code reviewVerdict: no bugs, crash paths, or token leaks found. One low-to-medium concern about the notice wording, posted inline. Bugs
Security
Performance
Quality
AGENTS.md
Earlier review threads
Change sizeCompared with
|
Summary
A cloud-optimised GeoTIFF tile layer saved by its plain web address loses its look.
saved_addresscuts everything after#from a plain address, and that part holds#range=…,#bands=…,#dequantize=aefand, since #1339,#cube=1. Saving then says "1 layer address(es) were saved without their keys, tokens or passwords", which is untrue here.urland in everytime_urlsentry.stac.addCubewrites its fixed"range"there, so a reopened cube stretches each file on its own and the same value no longer draws the same grey from date to date.time_urls, so#cube=1is all that keeps it a tile layer. Reading the code, onmainit is cut on saving and the layer opens as a raster.embeddings.add) lose their bands and stretch.map.addTilesopens as a raster since feat(raster): cloud-optimised GeoTIFFs are raster layers read in place #1339, without the look it was saved with.layer.addUrlof a plain COG is no longer affected: since #1339 it makes a raster layer, whose stretch is saved apart from its address.The doc comment on
saved_addresssays a COG's look is kept, andsplit_lookexists for that, but only the wrapper and template branches used it.Reproduced with this branch's tests and
project.rsfrommainat183ccdd3, savingmap.addTilesof a public COG with#range=0,1000:With this PR the same commands give
"stripped": 0, no such notice, and the address ends inTCI.tif#range=0,1000. Opened again, it is a raster layer stretched from 0 to 1000. With?token=s3cret&v=1before the look, the token goes, it is counted, and the address ends inTCI.tif?v=1#range=0,1000.Changes
crates/engine/src/project.rs,saved_address: the plain branch takes the look off withsplit_lookfirst, cleans the address as before, and puts the look back. An address with nothing else to remove now comes back unchanged, solost_secretno longer counts it andproject.saveAssays nothing about keys.crates/engine/src/project.rs,split_look: names what a saved look may hold:bandsandrange(whichLook::parsehas checked are numbers),dequantizeasaefornone,cubeas0or1. Anything else isn't kept.Look::parseaccepts any word fordequantizeandcube, and a look is saved as written, so#dequantize=<anything>would otherwise reach the file. This also closes that gap in the wrapper and template branches, which already kept looks as written. Reading is unchanged.docs/user-guide/workspace.md: says a look is saved and any other#part isn't.Choices in this proposal
#, assources::redactsays, so only a look stays: band numbers, a range of two numbers,aefornone.#access_token=…,#range=0,1&token=…and#range=0,1?token=…are not looks. In#sig=…#range=0,1only the last part stays.Look::suffix. Rewriting would turn#range=0.0,1000.0into#range=0,1000, the address would differ from the original, and it would be counted as a removed key.split_look, not a change toLook::parse, so what a file saves changes and how an address is read doesn't. A key added toLooklater isn't saved as written until it's named here: until then its look goes on saving, with the notice. RustGIS only ever writesaefandcube=1, andmap.setTilestakesaefornone.FORMATstays at 1. A saved look is mostly nothing new to a reader: v0.4.0 and v0.5.0 read#range=…,#bands=…and#dequantize=aefthroughcog::splitand draw them, and they already save looks on template and S3 connection addresses. v0.5.0 doesn't knowcube, so it reads an address holding it as having no look at all, which is what it would get from that address saved onmain. Saving again in those releases cuts the look and counts it in the "without their keys" notice, as they do today. Raising the version would make them refuse every project saved from now on, with or without a look, which seems too much for that. It's the maintainers' call.Known limits
public_web_addressstill removes every fragment from a plain address, so whichever merges second has to carry this rule over.#range=0,1&dequantize=otherstill draws with its range in the open project, but its whole fragment goes when saved, range included, and is counted in the notice. RustGIS never writes such an address.#range=0,1, say). It holds only numbers and these words.#section), since it can't be told from a token. Unchanged.sources::redact, used for the recent and saved sources lists, still drops every fragment, a look included. Its comment says nothing that adds a source needs it. Not changed here.Testing
tests::layer_address_redaction_keeps_a_cogs_look(engine): looks kept on plain,s3://, anonymous, login-bound and template addresses; keys removed around a look; fragments that resemble a look removed, on template and S3 connection addresses too.tests::saved_cubes_keep_their_look(engine): both ways throughproject.saveAs. A clean cube givesstripped0, no notice, and opens with the sameurlandtime_urls. A token on one date givesstripped1 and the notice, the file holds no token but keeps the look, and the open project keeps everything. A cube of one file opens with its#cube=1, as a tile layer.project.rsfrommain. With the closed list turned off, the first fails on#dequantize=s3cret.0bc424f2:cargo fmt --checkandcargo clippy -p rustgis-engine --all-targets -D warningspass, and so do therustgis-tiles,rustgis-mapandrustgis-datalib tests. Linux CI is the reference for the rest.rustgis-enginelib suite has no clean run on Windows here, with or without this change. Over 6 runs with it and 4 withproject.rsfrommain, the failures differ from run to run: 1 to 5 tests with it, 0 to 4 without (not counting this PR's two tests, which are expected to fail without it). They includepmtiles_/mbtiles_archives_frame_the_map_on_their_bounds,an_oversized_ogc_items_binding_is_refused_before_native_fetchand several tests feat(raster): cloud-optimised GeoTIFFs are raster layers read in place #1339 added, with errors such as "dropped: the view moved on" and "timeout: global". Tests run in parallel share the process's tile cache, which a session with logins clears when dropped.a_cog_read_in_place_from_a_connection_records_it_and_follows_its_loginfailed in 4 of the 6 runs with this change (http status: 401) and in none of the 4 without it. It passes alone, 3 of 3, and doesn't save a project.saved_addressonly returns a string and is reached only when saving, so I don't see a path from this change to that test, but the runs are too few to rule it out. Part of these runs shared the machine with another build.No visible change in the interface, so no screenshots.
🤖 Generated with Claude Code