search

tcplugins / tcWebHooks

WebHooks plugin for Teamcity. Supports many build states and payload formats.
https://netwolfuk.wordpress.com/category/teamcity/tcplugins/tcwebhooks/
157 stars 30 forks source link

Webhooks not showing when project is copied #238

Closed matt-richardson closed 5 months ago

matt-richardson commented 6 months ago

Expected Behavior

When copying a project, the new project should show the webhooks correctly.

Current Behavior

The webhooks will only show in one of the projects (it's not deterministic which one)

Steps to Reproduce (for bugs)

  1. Create a project (Project1)
  2. Setup a webhook on the project
  3. Copy the project (to Project2)
  4. view the webhooks for each project, webhooks will only be visible on one project

Problem

We copy projects on a semi-regular basis, to support our old "LTS" branches.

The new webhook has the same id as the old webhook, meaning that when it ends up in this map, we drop webhook instances.

This is similar to https://github.com/tcplugins/tcWebHooks/issues/10 - we'd need to deal with modifying the IDs during copy, but it still doesn't look like there's a nice way of being notified when a copy happens.

I looked to see what would happen if we dropped the uniqueid altogether, and it seems about half of the usages are around logging, but there's still a lot of spots that require some thinking.

🤔 An option could be to prefix the unique id with the project id when dealing with it in code, so the duplicate id's in the config dont matter.

🤔 Another option could be to drop the id from the config, and generate a hash of the projectid+the entire webhook config for use as the id?

Workaround

We can modify the id's manually in the config so they are unique, but it's not ideal.

Your Environment

netwolfuk commented 6 months ago

Hi @matt-richardson. How do you copy the projects? On the file system or via the UI (or REST API) or some other way?

The new webhook has the same id as the old webhook, meaning that when it ends up in this map, we drop webhook instances.

Ah, that makes sense. I wondered how that could happen as it might also be tangentially related to #232 Well spotted. I had forgotten about that map. :-(

Another option could be to drop the id from the config, and generate a hash of the projectid+the entire webhook config for use as the id?

I really like the ID in the XML file as it helps so much with debugging issues. Also, the ID there is the one used in the REST API to identify the webhook config.

I'll percolate on it. I'm always happy to hear your suggestions, as they are very well considered.

matt-richardson commented 6 months ago

How do you copy the projects? On the file system or via the UI (or REST API) or some other way?

We do it via the admin UI

image

netwolfuk commented 6 months ago

Thanks. Was just wondering in case I needed to do some de-duplication on changes based on file reloading. If that was the case, the id based on the project id would make a lot of sense.

netwolfuk commented 6 months ago

If the ID in that map is the problem, then combining the key in that map with the projectId would solve it. I was initially against it because I was thinking ProjectExternalId, which changes when the user modifies it. So I need to listen for those sorts of events somehow and update the cache.

But I just realised I could use the project internal ID as part of the key. I am thinking I could use a composite key object eg

  class WebHookCacheKey {
     private string projectInternalId;
     private String webhookId;
  }

rather than string munging when finding a webhook in the cache by ID.

matt-richardson commented 6 months ago

That'd work pretty well 👍

netwolfuk commented 6 months ago

Cool. I'll take a look later in the week or next weekend or sometime. PRs welcome of course :-)

My focus at the moment is getting #239 (as a result of #236) finalised. I don't have a working version on the plugins marketplace at the moment for 2024.03

netwolfuk commented 6 months ago

I have a working prototype. https://github.com/tcplugins/tcWebHooks/tree/issue_238-webhooks_not_showing_when_project_copied I'll do some tidy up tomorrow, and then add a link to a build for you to test.

netwolfuk commented 6 months ago

Build will be here (hopefully). https://teamcity.jetbrains.com/buildConfiguration/WebHooksAndOtherPlugins_TcWebHooks?branch=issue_238-webhooks_not_showing_when_project_copied&buildTypeTab=overview&mode=builds

netwolfuk commented 6 months ago

More updates. Much better handling of delete, archive, copy, de-archive of a project. Introduced a delayed listener that reacts to change events, waits 10 seconds and then re-reads the config. This is because project copy, and project de-archive take a while before the plugin-settings.xml is re-read.

netwolfuk commented 6 months ago

I think the only remaining thing is to go through and find duplicate IDs and rename them if they exist in two projects.

matt-richardson commented 6 months ago

Just gave it a spin: image

I'm running 2023.11 (build 147331).

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'jetbrains.buildServer.serverSide.BuildServerListenerEventDispatcher': Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.CannotLoadBeanClassException: Error loading class [webhook.teamcity.server.rest.jersey.WebHookSettingsManagerProvider] for bean with name 'webHookSettingsManagerProvider' defined in Byte array resource [plugin: tcWebHooks-rest-api#tcwebhooks-rest-api-2.0.1-rc.1-build.497-jar-with-dependencies.jar!/META-INF/build-server-plugin-RESTListener.xml]: problem with class file or dependent class; nested exception is java.lang.NoClassDefFoundError: jetbrains/buildServer/server/rest/jersey/provider/annotated/JerseyInjectableBeanProvider
at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:800)
at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:229)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.autowireConstructor(AbstractAutowireCapableBeanFactory.java:1372)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance(AbstractAutowireCapableBeanFactory.java:1222)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:582)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:542)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:378)
at jetbrains.buildServer.plugins.spring.PerPluginAdditionalSpringContextProvider.createEventDispatcherWrapper(PerPluginAdditionalSpringContextProvider.java:695)
at jetbrains.buildServer.plugins.spring.PerPluginAdditionalSpringContextProvider.registerServerEventDispatchers(PerPluginAdditionalSpringContextProvider.java:156)
at jetbrains.buildServer.plugins.spring.PerPluginAdditionalSpringContextProvider.registerEventDispatcherWrappers(PerPluginAdditionalSpringContextProvider.java:107)
at jetbrains.buildServer.plugins.spring.PerPluginAdditionalSpringContextProvider.updatePluginContext(PerPluginAdditionalSpringContextProvider.java:58)
at jetbrains.buildServer.plugins.spring.SpringPluginLoader$TeamCityPluginContext.prepareBeanFactory(SpringPluginLoader.java:276)
at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:556)
at jetbrains.buildServer.plugins.spring.SpringPluginLoader.pluginClassesLoaded(SpringPluginLoader.java:126)
at jdk.internal.reflect.GeneratedMethodAccessor82.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at jetbrains.buildServer.util.EventDispatcher.invokeListeners(EventDispatcher.java:157)
at jetbrains.buildServer.util.EventDispatcher.dispatch(EventDispatcher.java:136)
at jetbrains.buildServer.util.EventDispatcher$2.invoke(EventDispatcher.java:82)
at jdk.proxy3/jdk.proxy3.$Proxy47.pluginClassesLoaded(Unknown Source)
at jetbrains.buildServer.plugins.PluginManagerImpl$5.visitPlugin(PluginManagerImpl.java:470)
at jetbrains.buildServer.plugins.PluginsCollection.foreachLoadedPluginsAsync(PluginsCollection.java:114)
at jetbrains.buildServer.plugins.PluginManagerImpl.firePluginClassesLoaded(PluginManagerImpl.java:467)
at jetbrains.buildServer.plugins.PluginManagerImpl.doLoadPluginsCollection(PluginManagerImpl.java:410)
at jetbrains.buildServer.plugins.PluginManagerImpl.loadPlugins(PluginManagerImpl.java:378)
at jetbrains.buildServer.web.plugins.PluginManagerConfigurator.initializePlugins(PluginManagerConfigurator.java:59)
at jetbrains.buildServer.web.impl.BuildServerConfigurator.loadConfiguration(BuildServerConfigurator.java:102)
at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:807)
at jetbrains.buildServer.serverSide.impl.BuildServerLifecycleProcessor.doStartup(BuildServerLifecycleProcessor.java:64)
at jetbrains.buildServer.maintenance.TeamCityDispatcherServlet$WebApplicationCreatorAndDestroyer.createApplication(TeamCityDispatcherServlet.java:224)
at jetbrains.buildServer.maintenance.StartupProcessor.doApplicationStarting(StartupProcessor.java:2543)
at jetbrains.buildServer.maintenance.StartupProcessor.access$1800(StartupProcessor.java:115)
at jetbrains.buildServer.maintenance.StartupProcessor$3.call(StartupProcessor.java:323)
at jetbrains.buildServer.util.NamedThreadFactory.executeWithNewThreadName(NamedThreadFactory.java:91)
at jetbrains.buildServer.maintenance.StartupProcessor.processConcreteStage(StartupProcessor.java:302)
at jetbrains.buildServer.maintenance.StartupProcessor.processConcreteStageSafe(StartupProcessor.java:267)
at jetbrains.buildServer.maintenance.StartupProcessor.processTeamCityLifecycle(StartupProcessor.java:250)
at jetbrains.buildServer.maintenance.StartupProcessor.access$000(StartupProcessor.java:115)
at jetbrains.buildServer.maintenance.StartupProcessor$1.run(StartupProcessor.java:201)
at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: org.springframework.beans.factory.CannotLoadBeanClassException: Error loading class [webhook.teamcity.server.rest.jersey.WebHookSettingsManagerProvider] for bean with name 'webHookSettingsManagerProvider' defined in Byte array resource [plugin: tcWebHooks-rest-api#tcwebhooks-rest-api-2.0.1-rc.1-build.497-jar-with-dependencies.jar!/META-INF/build-server-plugin-RESTListener.xml]: problem with class file or dependent class; nested exception is java.lang.NoClassDefFoundError: jetbrains/buildServer/server/rest/jersey/provider/annotated/JerseyInjectableBeanProvider
at org.springframework.beans.factory.support.AbstractBeanFactory.resolveBeanClass(AbstractBeanFactory.java:1559)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.determineTargetType(AbstractAutowireCapableBeanFactory.java:704)
at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.predictBeanType(AbstractAutowireCapableBeanFactory.java:674)
at org.springframework.beans.factory.support.AbstractBeanFactory.isFactoryBean(AbstractBeanFactory.java:1684)
at org.springframework.beans.factory.support.DefaultListableBeanFactory.doGetBeanNamesForType(DefaultListableBeanFactory.java:570)
at org.springframework.beans.factory.support.DefaultListableBeanFactory.getBeanNamesForType(DefaultListableBeanFactory.java:542)
at org.springframework.beans.factory.BeanFactoryUtils.beanNamesForTypeIncludingAncestors(BeanFactoryUtils.java:265)
at org.springframework.beans.factory.support.DefaultListableBeanFactory.findAutowireCandidates(DefaultListableBeanFactory.java:1557)
at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1354)
at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1311)
at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:887)
at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:791)
... 40 more
Caused by: java.lang.NoClassDefFoundError: jetbrains/buildServer/server/rest/jersey/provider/annotated/JerseyInjectableBeanProvider
at java.base/java.lang.ClassLoader.defineClass1(Native Method)
at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
at java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:712)
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
at jetbrains.buildServer.plugins.classLoaders.HierarchicalPluginStandaloneClassLoaderImpl.doLoadClass(HierarchicalPluginStandaloneClassLoaderImpl.java:40)
at jetbrains.buildServer.plugins.classLoaders.TeamCityClassLoader.loadClass(TeamCityClassLoader.java:46)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:467)
at org.springframework.util.ClassUtils.forName(ClassUtils.java:284)
at org.springframework.beans.factory.support.AbstractBeanDefinition.resolveBeanClass(AbstractBeanDefinition.java:469)
at org.springframework.beans.factory.support.AbstractBeanFactory.doResolveBeanClass(AbstractBeanFactory.java:1621)
at org.springframework.beans.factory.support.AbstractBeanFactory.resolveBeanClass(AbstractBeanFactory.java:1548)
... 51 more
Caused by: java.lang.ClassNotFoundException: Class 'jetbrains.buildServer.server.rest.jersey.provider.annotated.JerseyInjectableBeanProvider' was not found
at jetbrains.buildServer.plugins.classLoaders.HierarchicalPluginStandaloneClassLoaderImpl.doLoadClass(HierarchicalPluginStandaloneClassLoaderImpl.java:69)
at jetbrains.buildServer.plugins.classLoaders.TeamCityClassLoader.loadClass(TeamCityClassLoader.java:46)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
... 68 more

Update Ahh, just realised that that would be a case of "I should be using the -legacy one instead". Odd though, as I thought I sawa it had a version compatibility check in the xml file?

Yep, that was it: image

netwolfuk commented 6 months ago

Yeah, that check is just for the marketplace it seems. I have already mentioned that to Jetbrains.

matt-richardson commented 6 months ago

Okay, doing some testing, it seems to work fine in the UI, but oddly, the persisted file on disk still refers to the original id.

netwolfuk commented 6 months ago

the persisted file on disk still refers to the original id.

Yeah, that's the last piece to fix.

netwolfuk commented 6 months ago

I realised in the middle of that night that keeping the key in the map with just the ID would work fine now. At the time that I write the webhook it into the map, I just need to do a contains on the keys, and then check if the projectId matches in the enhanced object.

Whereas now I need to iterate over every key and check if the unique id matches and project id doesn't.

netwolfuk commented 6 months ago

Arguably, it's not too much work on most systems. I ran a script and created thousands of webhooks to test the threading issues, so I probably have more webhooks than everyone else.

netwolfuk commented 6 months ago

I've removed that cache key, and added the re-keying logic. I'll kick off a build now.

netwolfuk commented 6 months ago

Sorry, forgot to update the version, so it's still 2.0.0-rc.1

matt-richardson commented 6 months ago

Gave it a quick spin - seems to work well.

I glanced through the code - happened to notice this code comment is no longer valid.

I have a niggling feeling that the 10 second delay might cause a bit of havoc - it seems ripe for race conditions. We use versioned settings, so I wonder if that might cause things to take longer than 10 seconds?

matt-richardson commented 6 months ago

At the risk of pushing my luck... I happen to know about a semi-related bug... if a webhook is set to run for specific build configs, and then that build config is deleted, the "project edit" page wont load. Want me to raise a new issue for that?

netwolfuk commented 6 months ago

Is it related to this change, or did it exist previously?

matt-richardson commented 6 months ago

Is it related to this change, or did it exist previously?

No, it existed previously

netwolfuk commented 6 months ago

I wonder if that might cause things to take longer than 10 seconds?

Pavel tells me that projectRestored will fire when the VCS config is reloaded. I've added support for that event too in this PR

netwolfuk commented 6 months ago

I presume it still exists or you wouldn't have mentioned it.

I'll look at it for this release. Is there any more detail I should know about how to reproduce it. If so please raise a new issue

netwolfuk commented 6 months ago 
    @Override
    public void projectRestored(String projectId) {
        Loggers.SERVER.debug("WebHookListener :: Handling projectRestored event for project: " + projectId);
        this.webHookSettingsEventHandler.handleEvent(WebHookSettingsEventType.PROJECT_CHANGED, projectId);
    }

I might need to change this, as you say it will introduce 10 second delay. I think I'll add a new state to the ENUM, and only wait a second or so on this, as I have been told that it fires after the change is read back in.

matt-richardson commented 6 months ago

I presume it still exists or you wouldn't have mentioned it. I'll look at it for this release. Is there any more detail I should know about how to reproduce it. If so please raise a new issue

Just reconfirmed it - I listed out the repro steps in https://github.com/tcplugins/tcWebHooks/issues/242 to make it as easy to track down as I could.

I suspect the fix could be related to this one - capturing an event when the build config is deleted and modifying the webhook there.

netwolfuk commented 5 months ago

Released https://github.com/tcplugins/tcWebHooks/releases/tag/v2.0.1