Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

docs: update 'corsEnabled' and explanation #680

Closed
wants to merge 1 commit into from
Closed

docs: update 'corsEnabled' and explanation #680

wants to merge 1 commit into from

Conversation

zjgoodman
Copy link

@zjgoodman zjgoodman commented Aug 4, 2021

the existing documentation calls the property cors-enabled but in actuality it's corsEnabled

also added a disclaimer about the behavior of the underlying cors properties

Background

I came across this behavior while using the spring boot graphql playground. The playground submits a POST to /graphql but I was getting cors errors. For the sake of local testing, I figured I would just turn off cors headers. Looking at the docs, I set cors-enabled to false but that didn't do anything. Then I tried corsEnabled and that did work. You can also see looking at this repo's source code that the code itself is using corsEnabled

Method I used for testing this

Note that I am using com.graphql-java-kickstart:graphql-spring-boot-starter:11.1.0

My graphql playground with cors enabled

Note that I am getting Invalid CORS request back from my spring boot app.
image

image

My graphql graphql playground with cors disabled

I disabled cors using corsEnabled: false. Note that I am getting 200 OK back from my spring boot app
image

the existing documentation calls the property 'cors-enabled' but in actuality it's 'corsEnabled'
@zjgoodman
Copy link
Author

@oliemansm lemme know if there's anything I need to do to get this merged in 😘

@BlasiusSecundus
Copy link

I am not sure if updating the docs is the correct approach in this case.

I am not sure if this is the expected / intended behaviour. I would expect that if I disable a feature then it will remain disabled, even if some properties related to the disabled feature are present.

@BlasiusSecundus
Copy link

BlasiusSecundus commented Aug 12, 2021

@ConditionalOnProperty(value = "graphql.servlet.corsEnabled", havingValue = "true", matchIfMissing = true)

Here @ConditionalOnProperty is used, which may not support relaxed binding. The documentation (https://docs.spring.io/spring-boot/docs/current/api/index.html?org/springframework/boot/autoconfigure/condition/ConditionalOnProperty.html) says:

Use the dashed notation to specify each property, that is all lower case with a "-" to separate words (e.g. my-long-property).

So in this case I think that the correct / "Spring-compliant" approach would be to change the property name to cors-enabled as advertised in the documentation, instead of updating the documentation.

@BlasiusSecundus BlasiusSecundus linked an issue Aug 12, 2021 that may be closed by this pull request
@zjgoodman
Copy link
Author

zjgoodman commented Aug 14, 2021

So in this case I think that the correct / "Spring-compliant" approach would be to change the property name to cors-enabled as advertised in the documentation, instead of updating the documentation.

Changing the property name in the code seems reasonable, but it would be a breaking change. I am interested in contributing the change but am a first time contributor unfamiliar with the process for handling a breaking change.

Is there a process for deprecating properties? A docs change would be a safer option, but if you think renaming the property is preferred then let’s go for it

@BlasiusSecundus
Copy link

The next release is a major version (12.0.0) already coming with some breaking changes (e. g. a major starter reorganization) - so in this case this breaking change may also get a pass - but should be documented in the changelogs properly.

Also, the commit may be marked as "breaking change" as described in the Conventional Commits specifications (https://www.conventionalcommits.org/en/v1.0.0/).

I don't think it is possible to deprecate it properly (also, matchIfMissing = true, making possible workarounds more difficult). As a @ConfigurationProperties member, both variants would work because of relaxed binding.

Checking other @ConditionalOnProperty usages in the project, the "-" (kebab case) convention is used consistently except for corsEnabled.

I would ask for a second opinion from @oliemansm on this topic, what would be the preferred approach.

@oliemansm
Copy link
Member

I agree that the kebab case is the preferred convention to be used. So it would be great @zjgoodman if you could make that so and use the breaking change commit message accordingly as @BlasiusSecundus mentioned.

@oliemansm
Copy link
Member

I've created a fix to support both versions of the property, so no more need for this PR. See: #732.

@oliemansm oliemansm closed this Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CorsEnabled=false not working for Spring-Boot-Service-Configs
4 participants