vikunja
/
vikunja
6
43
Fork 72
Code Issues 150 Pull Requests 19 Packages Releases 32 Activity

fix(image): json type of struct property #1469

Merged
konrad merged 3 commits from dpschen/api:fix-thumb-property-json-type into main 2023-04-06 09:53:11 +00:00
Conversation 11 Commits 3 Files Changed 6 +8 -8
dpschen commented 2023-04-05 10:39:27 +00:00
Member
Copy Link

I think this fixes some errors when validating the json (e.g. https://try.vikunja.io/api/v1/docs.json) via https://editor.swagger.io/.

Preparation for possibly using generators like https://github.com/astahmer/openapi-zod-client.
But also doesn't hurt in general. Will also help if we intend to switch so OpenApi 3 somehow.

I think this fixes some errors when validating the json (e.g. https://try.vikunja.io/api/v1/docs.json) via https://editor.swagger.io/. Preparation for possibly using generators like https://github.com/astahmer/openapi-zod-client. But also doesn't hurt in general. Will also help if we intend to switch so OpenApi 3 somehow.
konrad reviewed 2023-04-05 11:07:08 +00:00
pkg/modules/background/background.go Outdated
@ -27,3 +27,3 @@
ID string `json:"id"`
URL string `json:"url"`
Thumb string `json:"thumb,omitempty"`
Thumb string `json:"image,omitempty"`
konrad commented 2023-04-05 11:07:08 +00:00
Owner
Copy Link

This will change the name of the property in json. So with this change, the output will be

{
  "id": 42,
  ...
  "image": ...
}

Wheras right now it's this:

{
  "id": 42,
  ...
  "thumb": ...
}
This will change the name of the property in json. So with this change, the output will be ```json { "id": 42, ... "image": ... } ``` Wheras right now it's this: ```json { "id": 42, ... "thumb": ... } ```
✅ 1
dpschen commented 2023-04-05 13:04:41 +00:00
Author
Member
Copy Link

Removed

Removed
dpschen marked this conversation as resolved
konrad commented 2023-04-05 11:10:39 +00:00
Owner
Copy Link

I'm not sure if this property is even used. It looks like it isn't.

I'm not sure if this property is even used. It looks like it isn't.
konrad commented 2023-04-05 11:11:18 +00:00
Owner
Copy Link

The Image type is used for the search results when searching for unsplash images to set them as background.

The Image type is used for the search results when searching for unsplash images to set them as background.
dpschen commented 2023-04-05 11:47:00 +00:00
Author
Member
Copy Link

This is why I thought that this is the right thing to do:

image

image

This is why I thought that this is the right thing to do: ![image](/attachments/71ed0d35-e170-4bce-a85a-fab2ffcc2b3c) ![image](/attachments/6a29a64b-2c01-4642-9c3d-4045dc9a2f1e)
image.png
99 KiB
image.png
41 KiB
konrad commented 2023-04-05 11:49:33 +00:00
Owner
Copy Link

That needs to be changed here: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/background/unsplash/proxy.go#L52

That needs to be changed here: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/background/unsplash/proxy.go#L52
dpschen commented 2023-04-05 11:56:21 +00:00
Author
Member
Copy Link

That needs to be changed here: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/background/unsplash/proxy.go#L52

That makes sense! Thx

> That needs to be changed here: https://kolaente.dev/vikunja/api/src/branch/main/pkg/modules/background/unsplash/proxy.go#L52 That makes sense! Thx
dpschen force-pushed fix-thumb-property-json-type from acc572cb8e to 9cb3ecfd7d 2023-04-05 12:15:38 +00:00 Compare
dpschen commented 2023-04-05 13:05:05 +00:00
Author
Member
Copy Link

I pushed an updated version. Still very unsure

I pushed an updated version. Still very unsure
konrad added 2 commits 2023-04-06 09:36:27 +00:00
konrad approved these changes 2023-04-06 09:37:46 +00:00
konrad left a comment
Owner
Copy Link

The swagger editor seems to not complain about the file type anymore, looks like that was the problem.

The swagger editor seems to not complain about the file type anymore, looks like that was the problem.
konrad scheduled this pull request to auto merge when all checks succeed 2023-04-06 09:37:57 +00:00
konrad merged commit cca430810d into main 2023-04-06 09:53:11 +00:00
dpschen commented 2023-04-06 11:32:26 +00:00
Author
Member
Copy Link

I was mostly unsure about the second type. That's the golang type correct? Because e.g. for the unsplash proxy it was 'string' and I changed it to 'blob'.

They describe here

If the response returns the file alone, you would typically use a binary string schema and specify the appropriate media type for the response content:
[Code example]

That seems to be the correct way to do it? Should we change that? I could do it, but don't know where it should be defined.

I was mostly unsure about the second type. That's the golang type correct? Because e.g. for the unsplash proxy it was 'string' and I changed it to 'blob'. They describe [here](https://swagger.io/docs/specification/describing-responses/#response-that-returns-a-file) > If the response returns the file alone, you would typically use a binary string schema and specify the appropriate media type for the response content: [Code example] That seems to be the correct way to do it? Should we change that? I could do it, but don't know where it should be defined.
dpschen deleted branch fix-thumb-property-json-type 2023-04-06 11:32:43 +00:00
konrad commented 2023-04-06 11:52:12 +00:00
Owner
Copy Link

That seems to be the correct way to do it? Should we change that? I could do it, but don't know where it should be defined.

I think swaggo does not support that. The changes we did here in this PR are already a step forward and seem to work, so I'd say it's fine as it is now?

> That seems to be the correct way to do it? Should we change that? I could do it, but don't know where it should be defined. I think swaggo does not support that. The changes we did here in this PR are already a step forward and seem to work, so I'd say it's fine as it is now?
dpschen commented 2023-04-06 12:51:13 +00:00
Author
Member
Copy Link

I think it does via mime-type. Not really important and I'm also unsure if these would apply because ours are delivered as blob? Curious here, do you know?

Might be that the mentioned example is from OpenAPI 3 so it's not relevant.

and seem to work, so I'd say it's fine as it is now?

Agree!

I think it does via [mime-type](https://github.com/swaggo/swag#mime-types). Not really important and I'm also unsure if these would apply because ours are delivered as blob? Curious here, do you know? Might be that the mentioned example is from OpenAPI 3 so it's not relevant. > and seem to work, so I'd say it's fine as it is now? Agree!
👍 1
konrad commented 2023-04-06 12:53:43 +00:00
Owner
Copy Link

There might be different mime types returned because the api does not impose any restrictions on the format of uploaded backgrounds and will only return them as is. Not sure how that would be properly documented with openapi.

There might be different mime types returned because the api does not impose any restrictions on the format of uploaded backgrounds and will only return them as is. Not sure how that would be properly documented with openapi.
dpschen commented 2023-04-06 13:19:29 +00:00
Author
Member
Copy Link

There might be different mime types returned because the api does not impose any restrictions on the format of uploaded backgrounds and will only return them as is. Not sure how that would be properly documented with openapi.

Yes, that's what I meant. All good :)

> There might be different mime types returned because the api does not impose any restrictions on the format of uploaded backgrounds and will only return them as is. Not sure how that would be properly documented with openapi. Yes, that's what I meant. All good :)
Sign in to join this conversation.
Reviewers
No reviewers
konrad
Labels
Clear labels   
dependencies

   
duplicate

This issue or pull request already exists

  
help wanted

Need some help

  
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
needs reproduction

   
question

More information is needed

  
security

   
wontfix

This won't be fixed

No Label
dependencies
duplicate
help wanted
invalid
kind/bug
kind/feature
needs reproduction
question
security
wontfix
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#1469
No description provided.
Delete Branch "dpschen/api:fix-thumb-property-json-type"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?