Skip to content

Use ordered maps#695

Open
fenollp wants to merge 2 commits into
getkin:masterfrom
fenollp:issue645
Open

Use ordered maps#695
fenollp wants to merge 2 commits into
getkin:masterfrom
fenollp:issue645

Conversation

@fenollp
Copy link
Copy Markdown
Collaborator

@fenollp fenollp commented Dec 4, 2022

Fixes #645

@fenollp fenollp changed the title WIP needs un/marshalling Use ordered maps Dec 4, 2022
Comment thread openapi3/loader.go Fixed
Comment thread openapi3/loader.go Fixed
@fenollp fenollp force-pushed the issue645 branch 3 times, most recently from 391b9a7 to 1afb638 Compare December 19, 2022 16:59
@bitomaxsp
Copy link
Copy Markdown

bitomaxsp commented Dec 20, 2022

What kind of help is needed here? We (@northvolt) are super eager to see that released :)

@fenollp
Copy link
Copy Markdown
Collaborator Author

fenollp commented Dec 20, 2022

Hey @bitomaxsp I'd love some help! Right now I'm stuck solving this:

FAIL | Issue301 (0.00s)
     | panic: runtime error: invalid memory address or nil pointer dereference [recovered]
     | 	panic: runtime error: invalid memory address or nil pointer dereference
     | [signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x805850]
     | goroutine 19 [running]:
     | testing.tRunner.func1.2({0x888260, 0xc6abe0})
     | 	/snap/go/10008/src/testing/testing.go:1396 +0x24e
     | testing.tRunner.func1()
     | 	/snap/go/10008/src/testing/testing.go:1399 +0x39f
     | panic({0x888260, 0xc6abe0})
     | 	/snap/go/10008/src/runtime/panic.go:884 +0x212
     | github.com/getkin/kin-openapi/openapi3.TestIssue301(0x0?)
     | 	/home/pete/wefwefwef/kin-openapi.git/openapi3/issue301_test.go:32 +0x250
     | testing.tRunner(0xc0001ac820, 0x947b40)
     | 	/snap/go/10008/src/testing/testing.go:1446 +0x10b
     | created by testing.(*T).Run
     | 	/snap/go/10008/src/testing/testing.go:1493 +0x35f
FAIL | 	github.com/getkin/kin-openapi/openapi3	0.005s

From there I believe the design satisfies me. The rest of the work should be straightforward(TM)!

Could you describe your needs WRT this patch a bit?

Signed-off-by: Pierre Fenoll <pierrefenoll@gmail.com>
Signed-off-by: Pierre Fenoll <pierrefenoll@gmail.com>
@takanuva15
Copy link
Copy Markdown

Hi, did you have any luck fixing this issue? Looks like I encountered the same circular reference problem when trying to use this library with a large complex schema like in #814

@renom
Copy link
Copy Markdown

renom commented Aug 9, 2024

Is it going to be merged?

@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented Dec 30, 2025

Hey @fenollp 👋 (fun to see you somewhere else from buildkit 🎉)

Is there anything I can do to help move this along? Really interested in preserving the order with this, and there currently doesn't seem to be a way to do it without this.

@fenollp
Copy link
Copy Markdown
Collaborator Author

fenollp commented Dec 30, 2025

Hey @jedevc ! Cool to find you here as well :)

This whole PR is about switching all maps accesses to a method API (Set, Value, Iter, ...)
Also, to use the methods everywhere, drop the direct maps accesses, drop the usage of sort package. Split things in multiple PRs if possible...

This is just a bunch of work I haven't had the time to get around to. If you (or anyone) wants to go ahead, I'll review PRs!

@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented Apr 8, 2026

@fenollp sorry for taking a while to get to this 😅 I've spun out the first of a few, #1147. This does the sort removal that you suggested there.

@fenollp
Copy link
Copy Markdown
Collaborator Author

fenollp commented Apr 23, 2026

@jedevc PTAL at @latest of this package, I just merged #1151 & #1153 which may help your needs.

@jedevc
Copy link
Copy Markdown
Contributor

jedevc commented Apr 29, 2026

@fenollp #1153 is neat! One of the ones I'm struggling with is iterating over schema.Properties - which doesn't have a Keys method, so the iteration order is still random. Also, even with the Keys methods that are there, it's ordered lexigraphically, not in the original order, which IMO is a bit more useful, since then those properties are preserved.

I'm actually using kin-openapi as part of a text/template generation, which is quite neat, since a lot of the openapi generators I've tried out aren't quite what I want.

I think having an ordered map is still the right call for this, sorry for the delays in working on this!

@fenollp
Copy link
Copy Markdown
Collaborator Author

fenollp commented Apr 29, 2026

the original order
Well yes that's the end goal here and this PR started just that: using orderedmap at deserialization. componentNames is ducktape until then to preserve some sanity.

I am just wondering how displeased users will be if/when the whole lib moves to accessing things through methods (.Value(), .Keys(), ...) instead of "directly" (mymap["mykey"]).
Maybe there's a way to provide both APIs concurrently and to leave users the choice: do I want "easy programming" or do I want deterministic programs?

@fenollp
Copy link
Copy Markdown
Collaborator Author

fenollp commented Apr 29, 2026

@jedevc Hey by the way this is 4 years old. It can wait. Don't burn yourself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

please user OrderedMap instead of the golang's map

6 participants