Use `@CrossOrigin` annotations instead of delegating CORS configuration to properties files

trebol-ecommerce / trebol-backend-monolith

Monolithic eCommerce backend web application that exposes a RESTful API.

MIT License

16 stars 21 forks source link

Use `@CrossOrigin` annotations instead of delegating CORS configuration to properties files #104

Closed bglamadrid closed 1 year ago

bglamadrid commented 2 years ago

Apparently I didn't know but Spring Web MVC so happens to support annotation-based CORS configuration on a per-class and per-method basis. These are then added to the default CorsConfiguration bean.

Additional considerations

This may allow some classes to be released (as in, removed from the codebase) with ease
- org.trebol.config.CorsConfigurationBuilder
- org.trebol.config.CorsProperties
- org.trebol.exceptions.CorsMappingParseException

mepox commented 2 years ago

* This may allow some classes to be released (as in, removed from the codebase) with ease

  * `org.trebol.config.CorsConfigurationBuilder`
  * `org.trebol.config.CorsProperties`

Then the org.trebol.exceptions.CorsMappingParseException could be removed too?

bglamadrid commented 2 years ago

* This may allow some classes to be released (as in, removed from the codebase) with ease

  * `org.trebol.config.CorsConfigurationBuilder`
  * `org.trebol.config.CorsProperties`

Then the org.trebol.exceptions.CorsMappingParseException could be removed too?

That's right. That exception is only thrown within CorsConfigurationBuilder.

mepox commented 1 year ago

Hi @bglamadrid ! I am looking at this one for a while to do next, but I have a few questions first:

Is it really necessary? 😅 What would be the benefit of using this annotation and add it to each controller? I mean wouldn't be better to have these CORS config at one place? As far as I understand you would have to add this annotation to each controller class to configure the allowed request methods and (possibly?) also the origins which would be the same for all the controller anyway? If the origins changes you would have to change it in all controller?

bglamadrid commented 1 year ago

Hi @mepox. Let's see. None of these questions I have the definitive, ultimate answer to. It's a matter of picking the best tradeoffs.

So for example, having all CORS configuration in the .properties file makes it easy to update it through other means supported by Spring, such as environment variables. Plus, it's a single source of truth. But if for any reason you decide to modify API paths e.g. changing /sales to /orders, then you also have to update the CORS mappings for them yourself.

On the other side, adding annotations to Controller classes and methods binds CORS mappings to the actual API paths. No need to update anything else. But you do have to know that CORS is enabled thanks to those annotations. And annotations can be parameterized too. You should know this well since you created the PhoneNumberValidator and its annotation 😉

mepox commented 1 year ago

Hi @bglamadrid ! Thanks for your detailed reply. 😄

On the other side, adding annotations to Controller classes and methods binds CORS mappings to the actual API paths. No need to update anything else.

This is what I was looking for, I missed it... That's explain the benefit of using this annotation. 👍 I would like to work on this issue.